Hi Segher,
[Sorry to be answering these out of order...]
On 11/5/21 5:37 PM, Segher Boessenkool wrote:
> On Wed, Sep 01, 2021 at 11:13:52AM -0500, Bill Schmidt wrote:
>> * gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Adjust.
> My favourite changelog entry! But, adjust to what? This is the first
> line :-)
>
> "Adjust expected error message"?
OK, I'll be a bit less succinct. :-)
>
> But you should fold this patch with some previous patch anyway, when
> committing (or you break bisecting).
Yes, I failed to mention that patches 15-17 need to go in together to avoid
bisection problems.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/fold-vec-splat-floatdouble.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-splat-floatdouble.c
>> @@ -18,7 +18,7 @@ vector float test_fc ()
>> vector double testd_00 (vector double x) { return vec_splat (x, 0b00000); }
>> vector double testd_01 (vector double x) { return vec_splat (x, 0b00001); }
>> vector double test_dc ()
>> -{ const vector double y = { 3.0, 5.0 }; return vec_splat (y, 0b00010); }
>> +{ const vector double y = { 3.0, 5.0 }; return vec_splat (y, 0b00001); }
>>
>> /* If the source vector is a known constant, we will generate a load or
>> possibly
>> XXSPLTIW. */
>> @@ -28,5 +28,5 @@ vector double test_dc ()
>> /* { dg-final { scan-assembler-times {\mvspltw\M|\mxxspltw\M} 3 } } */
>>
>> /* For double types, we will generate xxpermdi instructions. */
>> -/* { dg-final { scan-assembler-times "xxpermdi" 3 } } */
>> +/* { dg-final { scan-assembler-times "xxpermdi" 2 } } */
> Why these changes?
Sorry, I should have done a better job of explaining these. For vector
double, only one bit matters, so the bit mask 0b00010 is a nonsensical
thing to have in the test case. Replacing that with 0b00001 resulted
in one fewer xxpermdi required. I'm going to review this one more time
to remind myself why, since I made this change a long time ago and it's
not fresh in my mind; it made sense then! :-)
>
>> --- a/gcc/testsuite/gcc.target/powerpc/fold-vec-splat-longlong.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-splat-longlong.c
>> @@ -9,23 +9,19 @@
>>
>> vector bool long long testb_00 (vector bool long long x) { return vec_splat
>> (x, 0b00000); }
>> vector bool long long testb_01 (vector bool long long x) { return vec_splat
>> (x, 0b00001); }
>> -vector bool long long testb_02 (vector bool long long x) { return vec_splat
>> (x, 0b00010); }
>>
>> vector signed long long tests_00 (vector signed long long x) { return
>> vec_splat (x, 0b00000); }
>> vector signed long long tests_01 (vector signed long long x) { return
>> vec_splat (x, 0b00001); }
>> -vector signed long long tests_02 (vector signed long long x) { return
>> vec_splat (x, 0b00010); }
>>
>> vector unsigned long long testu_00 (vector unsigned long long x) { return
>> vec_splat (x, 0b00000); }
>> vector unsigned long long testu_01 (vector unsigned long long x) { return
>> vec_splat (x, 0b00001); }
>> -vector unsigned long long testu_02 (vector unsigned long long x) { return
>> vec_splat (x, 0b00010); }
>>
>> /* Similar test as above, but the source vector is a known constant. */
>> -vector bool long long test_bll () { const vector bool long long y = {12,
>> 23}; return vec_splat (y, 0b00010); }
>> -vector signed long long test_sll () { const vector signed long long y =
>> {34, 45}; return vec_splat (y, 0b00010); }
>> -vector unsigned long long test_ull () { const vector unsigned long long y =
>> {56, 67}; return vec_splat (y, 0b00010); }
>> +vector bool long long test_bll () { const vector bool long long y = {12,
>> 23}; return vec_splat (y, 0b00001); }
>> +vector signed long long test_sll () { const vector signed long long y =
>> {34, 45}; return vec_splat (y, 0b00001); }
>>
>> /* Assorted load instructions for the initialization with known constants.
>> */
>> -/* { dg-final { scan-assembler-times {\mlvx\M|\mlxvd2x\M|\mlxv\M|\mplxv\M}
>> 3 } } */
>> +/* { dg-final { scan-assembler-times
>> {\mlvx\M|\mlxvd2x\M|\mlxv\M|\mplxv\M|\mxxspltib\M} 2 } } */
>>
>> /* xxpermdi for vec_splat of long long vectors.
>> At the time of this writing, the number of xxpermdi instructions
> Ditto.
Same issue. 0b00010 makes no sense for vector long long. I need to remind
myself about the change in counts here as well.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c
>> @@ -11,9 +11,9 @@
>> /* { dg-final { scan-assembler-times {\mvrlq\M} 2 } } */
>> /* { dg-final { scan-assembler-times {\mvrlqnm\M} 2 } } */
>> /* { dg-final { scan-assembler-times {\mvrlqmi\M} 2 } } */
>> -/* { dg-final { scan-assembler-times {\mvcmpequq\M} 16 } } */
>> -/* { dg-final { scan-assembler-times {\mvcmpgtsq\M} 16 } } */
>> -/* { dg-final { scan-assembler-times {\mvcmpgtuq\M} 16 } } */
>> +/* { dg-final { scan-assembler-times {\mvcmpequq\M} 24 } } */
>> +/* { dg-final { scan-assembler-times {\mvcmpgtsq\M} 26 } } */
>> +/* { dg-final { scan-assembler-times {\mvcmpgtuq\M} 26 } } */
>> /* { dg-final { scan-assembler-times {\mvmuloud\M} 1 } } */
>> /* { dg-final { scan-assembler-times {\mvmulesd\M} 1 } } */
>> /* { dg-final { scan-assembler-times {\mvmulosd\M} 1 } } */
> And this?
Again I'm a little sketchy on the details, but I believe this resulted
from some of the vector compares having been previously omitted by
accident from gimple expansion. When I added them in for the new
support, that gave us increased counts here because the code generation
was improved. I'll double-check this one as well to provide a more
certain explanation.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/p8vector-builtin-8.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/p8vector-builtin-8.c
>> @@ -126,6 +126,7 @@ void foo (vector signed char *vscr,
>> /* { dg-final { scan-assembler-times "vsubcuw" 4 } } */
>> /* { dg-final { scan-assembler-times "vsubuwm" 4 } } */
>> /* { dg-final { scan-assembler-times "vbpermq" 2 } } */
>> +/* { dg-final { scan-assembler-times "vbpermd" 0 } } */
>> /* { dg-final { scan-assembler-times "xxleqv" 4 } } */
>> /* { dg-final { scan-assembler-times "vgbbd" 1 } } */
>> /* { dg-final { scan-assembler-times "xxlnand" 4 } } */
> This curious one could have been a separate (obvious) patch. It is a
> bit out-of-place here.
Yeah, bit of a head-scratcher, this. The test case probably went
through a few revisions. I'll test it once more and commit it
separately.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/pragma_power8.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/pragma_power8.c
>> @@ -19,6 +19,7 @@ test1 (vector int a, vector int b)
>> #pragma GCC target ("cpu=power7")
>> /* Force a re-read of altivec.h with new cpu target. */
>> #undef _ALTIVEC_H
>> +#undef _RS6000_VECDEFINES_H
>> #include <altivec.h>
> Wow ugly :-) But nothing new here, heh. Best not to look at testcase
> internals too closely, in any case.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/test_mffsl.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/test_mffsl.c
>> @@ -1,5 +1,6 @@
>> /* { dg-do run { target { powerpc*-*-* } } } */
>> -/* { dg-options "-O2 -std=c99" } */
>> +/* { dg-options "-O2 -std=c99 -mcpu=power9" } */
>> +/* { dg-require-effective-target powerpc_p9vector_ok } */
>>
>> #ifdef DEBUG
>> #include <stdio.h>
> This one is a bug fix as well (and obvious).
Yeah. :-( Will handle.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/vsu/vec-all-nez-7.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/vsu/vec-all-nez-7.c
>> @@ -12,5 +12,5 @@ test_all_not_equal_and_not_zero (vector unsigned short
>> *arg1_p,
>> vector unsigned short arg_2 = *arg2_p;
>>
>> return __builtin_vec_vcmpnez_p (__CR6_LT, arg_1, arg_2);
>> - /* { dg-error "'__builtin_altivec_vcmpnezh_p' requires the '-mcpu=power9'
>> option" "" { target *-*-* } .-1 } */
>> + /* { dg-error "'__builtin_altivec_vcmpnezh_p' requires the
>> '-mpower9-vector' option" "" { target *-*-* } .-1 } */
>> }
> Hrm. People should not use the -mpower9-vector option (except implied
> by -mcpu=power9, without vectors disabled). How hard is it to give a
> better error message here?
Yeah, agreed, I think I can fix that easily enough. There may be similar
issues with -mpower8-vector as well that should be fixed.
Thanks for the review! I'll get back on this one soon.
Bill
>
> The obvious bugfixes independent of this series are of course okay for
> trunk, as separate patches, now. But some more work is needed
> elsewhere.
>
>
> Segher
