Hi Carl,
On Thu, Jan 18, 2018 at 08:06:17AM -0800, Carl Love wrote:
> The following patch adds missing 128-bit support for the builtins
> vec_xl(), vec_xl_be(), vec_xst(), vec_xst_be(). It also includes a bug
> fix required for the new 128-bit arguments for the vec_xst_be() and
> vec_xl_be() builtins. New test cases are also included. This patch
> completes the tests of all the various load/store builtins that I have
> been working on. This patch adds a torture test for the various load
> store tests to check that they work for -O0, -O1 and -O2 testing. This
> was done as the work on the load/store builtins found numerous issues
> that were optimization level dependent which were fixed by the previous
> commits as well as this commit.
> Let me know if the patch looks OK or not. Let me know if you want to
> include it in stage 4 or wait for the next release. Thanks.
Since this is last I think we should include it now; it isn't likely
to regress anything, either.
But, some comments, and a real bug:
> 2018-01-18 Carl Love <c...@us.ibm.com>
> * gcc.target/powerpc/powerpc.exp: Add torture tests for
> builtins-4-runnable.c, builtins-6-runnable.c,
> builtins-5-p9-runnable.c, builtins-6-p9-runnable.c.
The indent here is wrong (stray space).
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -15572,6 +15572,12 @@ altivec_expand_builtin (tree exp, rtx target, bool
> *expandedp)
> unaligned-supporting store, so use a generic expander. For
> little-endian, the exact element-reversing instruction must
> be used. */
> + case VSX_BUILTIN_ST_ELEMREV_V1TI:
> + {
> + enum insn_code code = (BYTES_BIG_ENDIAN ? CODE_FOR_vsx_store_v1ti
> + : CODE_FOR_vsx_st_elemrev_v1ti);
> + return altivec_expand_stv_builtin (code, exp);
> + }
Last line has a space too many. Or, actually, all the rest have one
too few?
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -1093,6 +1093,18 @@ (define_insn "vsx_ld_elemrev_v2di"
> "lxvd2x %x0,%y1"
> [(set_attr "type" "vecload")])
>
> +(define_insn "vsx_ld_elemrev_v1ti"
> + [(set (match_operand:V1TI 0 "vsx_register_operand" "=wa")
> + (vec_select:V1TI
> + (match_operand:V1TI 1 "memory_operand" "Z")
> + (parallel [(const_int 0)])))]
> + "VECTOR_MEM_VSX_P (V1TImode) && !BYTES_BIG_ENDIAN"
> +{
> + return "lxvd2x %x0,%y1; xxpermdi %x0,%x0,%x0,2";
> +}
Not ; but \; please. No space after it.
We currently have exactly as many xxpermdi,2 as xxswapdi (147 each)
but the latter is more readable, please prefer that.
> +(define_insn "vsx_st_elemrev_v1ti"
> + [(set (match_operand:V1TI 0 "memory_operand" "=Z")
> + (vec_select:V1TI
> + (match_operand:V1TI 1 "vsx_register_operand" "wa")
> + (parallel [(const_int 0)])))]
> + "VECTOR_MEM_VSX_P (V2DImode) && !BYTES_BIG_ENDIAN"
> +{
> + return "xxpermdi %x1,%x1,%x1,2; stxvd2x %x1,%y0";
> +}
This is wrong: it changes operand 1 but the RTL does not mention that.
> --- a/gcc/testsuite/gcc.target/powerpc/powerpc.exp
> +++ b/gcc/testsuite/gcc.target/powerpc/powerpc.exp
> @@ -49,4 +49,16 @@ gcc-dg-runtest [list $srcdir/$subdir/savres.c] "" $alti
>
> # All done.
> torture-finish
> +
> +torture-init
> +# Test load/store builtins at all optimizations
> +set-torture-options [list -O0 -O1 -O2]
All? -O3 and -Os as well? (And maybe more, but those are important).
> +gcc-dg-runtest [list $srcdir/$subdir/builtins-4-runnable.c \
> + $srcdir/$subdir/builtins-6-runnable.c \
> + $srcdir/$subdir/builtins-5-p9-runnable.c \
> + $srcdir/$subdir/builtins-6-p9-runnable.c] ""
> $DEFAULT_CFLAGS
Weird indent on this line.
Rest looks fine :-)
Segher
