On 11/17/21 6:44 AM, Segher Boessenkool wrote:
> Hi!
>
> On Tue, Nov 16, 2021 at 02:26:22PM -0600, Bill Schmidt wrote:
>> Hi! I recently submitted [1] to make adjustments to test cases for the new
>> builtins
>> support, mostly due to error messages changing for consistency. Thanks for
>> the
>> previous review. I've reviewed the reasons for the changes and removed
>> unrelated
>> changes as requested.
> And the results are? This is much easier to write up, and to review, if
> you split the patch into pieces with one theme each. If you do that
> right then most reviews will be rubber-stamping, and some might require
> some thought (and some may even get objections). The way things are it
> is a puzzle hunt to review this.
Sorry! I thought I was addressing the issues that came up last time. I didn't
intend for this to be difficult. I will break the patch up going forward.
>
>> - For fold-vect-splat-floatdouble.c and fold-vec-splat-longlong.c, the
>> existing
>> test cases have some bad tests in them (checking two bits when only one
>> bit
>> is meaningful). The new builtin support catches this but the old support
>> did
>> not. Removing those bad cases changes some of the scan-assembler-times
>> expected
>> values.
> Do this is a separate patch then, independent of the series? With this
> explanation in the commit message. This is pre-approved.
OK, will do.
>
>> - For int_128bit-runnable.c, I chose not to do gimple folding on the 128-bit
>> comparison operations in the new implementation, because doing so results
>> in
>> bad code that splits things into two 64-bit values. That needs separate
>> attention; but the point here is, when I did that, I started generating
>> more of the vcmpequq, vcmpgtsq, and vcmpgtuq instructions.
> And you now get worse code (albeit in some cases no longer invalid)?
No, sorry that this wasn't more clear. The "old" builtins code performs
gimple folding on 128-bit compares. This results in correct but very
inefficient code. The "new" builtins code has removed the gimple folding
for 128-bit compares. This results in directly generating vcmpequq and
friends, which is the efficient code we're looking for. This test case
then needs modification to show we're doing better. I'll submit this
separately.
>
>
>> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c
>> @@ -14,7 +14,7 @@ get_exponent (double *p)
>> {
>> double source = *p;
>>
>> - return scalar_extract_exp (source); /* { dg-error
>> "'__builtin_vec_scalar_extract_exp' is not supported in this compiler
>> configuration" } */
>> + return scalar_extract_exp (source); /* { dg-error
>> "'__builtin_vsx_scalar_extract_exp' requires the" } */
>> }
> The testcase uses __builtin_vec_scalar_extract_exp, so this is not okay.
Sorry, this is a case of my bad eyesight not identifying this had changed.
As with the test case (cmpb-3.c) in the 32-bit patch, this error message
isn't all that the user sees. There is also a "note" diagnostic that ties
the generic overload name to the specific underlying builtin name so that
confusion is avoided. I'll just submit these separately with a full
explanation.
Same applies to the similar cases below.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-2.c
>> @@ -12,5 +12,5 @@ get_significand (double *p)
>> {
>> double source = *p;
>>
>> - return __builtin_vec_scalar_extract_sig (source); /* { dg-error
>> "'__builtin_vec_scalar_extract_sig' is not supported in this compiler
>> configuration" } */
>> + return __builtin_vec_scalar_extract_sig (source); /* { dg-error
>> "'__builtin_vsx_scalar_extract_sig' requires the" } */
>> }
> This not either.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-2.c
>> @@ -16,5 +16,5 @@ insert_exponent (unsigned long long int *significand_p,
>> unsigned long long int significand = *significand_p;
>> unsigned long long int exponent = *exponent_p;
>>
>> - return scalar_insert_exp (significand, exponent); /* { dg-error
>> "'__builtin_vec_scalar_insert_exp' is not supported in this compiler
>> configuration" } */
>> + return scalar_insert_exp (significand, exponent); /* { dg-error
>> "'__builtin_vsx_scalar_insert_exp' requires the" } */
> Or this.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-5.c
>> @@ -16,5 +16,5 @@ insert_exponent (double *significand_p,
>> double significand = *significand_p;
>> unsigned long long int exponent = *exponent_p;
>>
>> - return scalar_insert_exp (significand, exponent); /* { dg-error
>> "'__builtin_vec_scalar_insert_exp' is not supported in this compiler
>> configuration" } */
>> + return scalar_insert_exp (significand, exponent); /* { dg-error
>> "'__builtin_vsx_scalar_insert_exp_dp' requires the" } */
>> }
> Etc.
>
> It is not okay to blindly adjust the testcases to accept what the new
> code does. This is a regression. It is okay to have it regressed for a
> while. It is also okay to xfail things, if there is no expectation it
> can be fixed before the next release (or some other suitably big time
> frame, this isn't an exact science).
This isn't really a regression, as I'll describe with each patch.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-test-neg-2.c
>> @@ -10,5 +10,5 @@ test_neg (float *p)
>> {
>> float source = *p;
>>
>> - return __builtin_vec_scalar_test_neg_sp (source); /* { dg-error
>> "'__builtin_vsx_scalar_test_neg_sp' requires" } */
>> + return __builtin_vec_scalar_test_neg (source); /* { dg-error
>> "'__builtin_vsx_scalar_test_neg_sp' requires" } */
>> }
> This one is very curious. You change the test to use a more generic
> builtin name, presumably because the (undocumented) more specific name
> is no longer allowed, but the error message still uses that name?
I need to review this one. There were some deprecated interfaces involved here.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/byte-in-set-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/byte-in-set-2.c
>> @@ -10,5 +10,5 @@
>> int
>> test_byte_in_set (unsigned char b, unsigned long long set_members)
>> {
>> - return __builtin_byte_in_set (b, set_members); /* { dg-warning "implicit
>> declaration of function" } */
>> + return __builtin_byte_in_set (b, set_members); /* { dg-error
>> "'__builtin_scalar_byte_in_set' requires the" } */
>> }
> Huh. How can the old warning ever have fired? Was the builtin not
> declared on 32-bit before? Ouch.
I'll remind myself what changed here, but yes, that's what it looks like --
an inadvertent problem with the old logic for 32-bit.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/cmpb-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/cmpb-2.c
>> @@ -8,7 +8,7 @@ void abort ();
>> unsigned long long int
>> do_compare (unsigned long long int a, unsigned long long int b)
>> {
>> - return __builtin_cmpb (a, b); /* { dg-warning "implicit declaration
>> of function '__builtin_cmpb'" } */
>> + return __builtin_cmpb (a, b); /* { dg-error "'__builtin_p6_cmpb'
>> requires the '-mcpu=power6' option" } */
>> }
> We talked about this one before already.
Yes, although it was for cmpb-3.c in the 32-bit patch. I explained what's
going on there with the "note" diagnostic.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/crypto-builtin-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/crypto-builtin-2.c
>> @@ -5,21 +5,21 @@
>>
>> void use_builtins_d (__vector unsigned long long *p, __vector unsigned long
>> long *q, __vector unsigned long long *r, __vector unsigned long long *s)
>> {
>> - p[0] = __builtin_crypto_vcipher (q[0], r[0]); /* { dg-error
>> "'__builtin_crypto_vcipher' is not supported with the current options" } */
>> - p[1] = __builtin_crypto_vcipherlast (q[1], r[1]); /* { dg-error
>> "'__builtin_crypto_vcipherlast' is not supported with the current options" }
>> */
>> - p[2] = __builtin_crypto_vncipher (q[2], r[2]); /* { dg-error
>> "'__builtin_crypto_vncipher' is not supported with the current options" } */
>> - p[3] = __builtin_crypto_vncipherlast (q[3], r[3]); /* { dg-error
>> "'__builtin_crypto_vncipherlast' is not supported with the current options"
>> } */
>> + p[0] = __builtin_crypto_vcipher (q[0], r[0]); /* { dg-error
>> "'__builtin_crypto_vcipher' requires the '-mcrypto' option" } */
>> + p[1] = __builtin_crypto_vcipherlast (q[1], r[1]); /* { dg-error
>> "'__builtin_crypto_vcipherlast' requires the '-mcrypto' option" } */
>> + p[2] = __builtin_crypto_vncipher (q[2], r[2]); /* { dg-error
>> "'__builtin_crypto_vncipher' requires the '-mcrypto' option" } */
>> + p[3] = __builtin_crypto_vncipherlast (q[3], r[3]); /* { dg-error
>> "'__builtin_crypto_vncipherlast' requires the '-mcrypto' option" } */
>> p[4] = __builtin_crypto_vpermxor (q[4], r[4], s[4]);
>> p[5] = __builtin_crypto_vpmsumd (q[5], r[5]);
>> - p[6] = __builtin_crypto_vshasigmad (q[6], 1, 15); /* { dg-error
>> "'__builtin_crypto_vshasigmad' is not supported with the current options" }
>> */
>> - p[7] = __builtin_crypto_vsbox (q[7]); /* { dg-error
>> "'__builtin_crypto_vsbox' is not supported with the current options" } */
>> + p[6] = __builtin_crypto_vshasigmad (q[6], 1, 15); /* { dg-error
>> "'__builtin_crypto_vshasigmad' requires the '-mcrypto' option" } */
>> + p[7] = __builtin_crypto_vsbox (q[7]); /* { dg-error
>> "'__builtin_crypto_vsbox' requires the '-mcrypto' option" } */
>> }
>>
>> void use_builtins_w (__vector unsigned int *p, __vector unsigned int *q,
>> __vector unsigned int *r, __vector unsigned int *s)
>> {
>> p[0] = __builtin_crypto_vpermxor (q[0], r[0], s[0]);
>> p[1] = __builtin_crypto_vpmsumw (q[1], r[1]);
>> - p[2] = __builtin_crypto_vshasigmaw (q[2], 1, 15); /* { dg-error
>> "'__builtin_crypto_vshasigmaw' is not supported with the current options" }
>> */
>> + p[2] = __builtin_crypto_vshasigmaw (q[2], 1, 15); /* { dg-error
>> "'__builtin_crypto_vshasigmaw' requires the '-mcrypto' option" } */
>> }
> This one is fine.
>
>> --- 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 } } */
> This is okay as a separate patch, with proper commit message.
>
>> --- 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 } } */
> Ditto.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/fold-vec-splat-misc-invalid.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-splat-misc-invalid.c
>> @@ -10,24 +10,24 @@
>> vector signed short
>> testss_1 (unsigned int ui)
>> {
>> - return vec_splat_s16 (ui);/* { dg-error "argument 1 must be a 5-bit
>> signed literal" } */
>> + return vec_splat_s16 (ui);/* { dg-error "argument 1 must be a literal
>> between -16 and 15, inclusive" } */
>> }
> All such things are fine of course.
>
>> --- 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 } } */
> This either needs more explanation (and be a separate patch), or it is
> just wrong :-(
This is the gimple-fold issue discussed at the top. Will submit
separately.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/pr80315-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr80315-2.c
>> @@ -10,6 +10,6 @@ main ()
>> int mask;
>>
>> /* Argument 2 must be 0 or 1. Argument 3 must be in range 0..15. */
>> - res = __builtin_crypto_vshasigmad (test, 1, 0xff); /* { dg-error
>> {argument 3 must be in the range \[0, 15\]} } */
>> + res = __builtin_crypto_vshasigmad (test, 1, 0xff); /* { dg-error
>> {argument 3 must be a 4-bit unsigned literal} } */
>> return 0;
>> }
> Hrm, make this say "must be a literal between 0 and 15, inclusive" like
> the other errors?
The "n-bit unsigned literal" is the usual case. I'll provide more explanation
in the separate patch.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin_error.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin_error.c
>> @@ -8,13 +8,13 @@ int main ()
>> int arguments. The builtins __builtin_set_fpscr_rn() also supports a
>> variable as an argument but can't test variable value at compile time.
>> */
>>
>> - __builtin_mtfsb0(-1); /* { dg-error "Argument must be a constant between
>> 0 and 31" } */
>> - __builtin_mtfsb0(32); /* { dg-error "Argument must be a constant between
>> 0 and 31" } */
>> + __builtin_mtfsb0(-1); /* { dg-error "argument 1 must be a 5-bit unsigned
>> literal" } */
>> + __builtin_mtfsb0(32); /* { dg-error "argument 1 must be a 5-bit unsigned
>> literal" } */
>>
>> - __builtin_mtfsb1(-1); /* { dg-error "Argument must be a constant between
>> 0 and 31" } */
>> - __builtin_mtfsb1(32); /* { dg-error "Argument must be a constant between
>> 0 and 31" } */
>> + __builtin_mtfsb1(-1); /* { dg-error "argument 1 must be a 5-bit unsigned
>> literal" } */
>> + __builtin_mtfsb1(32); /* { dg-error "argument 1 must be a 5-bit unsigned
>> literal" } */
>>
>> - __builtin_set_fpscr_rn(-1); /* { dg-error "Argument must be a value
>> between 0 and 3" } */
>> - __builtin_set_fpscr_rn(4); /* { dg-error "Argument must be a value
>> between 0 and 3" } */
>> + __builtin_set_fpscr_rn(-1); /* { dg-error "argument 1 must be a variable
>> or a literal between 0 and 3, inclusive" } */
>> + __builtin_set_fpscr_rn(4); /* { dg-error "argument 1 must be a variable
>> or a literal between 0 and 3, inclusive" } */
>> }
> This regressed as well.
I don't think it's a regression, just a better error message. Will add
explanation
and submit separately.
>
>> --- a/gcc/testsuite/gcc.target/powerpc/vec-gnb-2.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/vec-gnb-2.c
>> @@ -20,7 +20,7 @@ do_vec_gnb (vector unsigned __int128 source, int stride)
>> case 5:
>> return vec_gnb (source, 1); /* { dg-error "between 2 and 7" } */
>> case 6:
>> - return vec_gnb (source, stride); /* { dg-error "unsigned
>> literal" } */
>> + return vec_gnb (source, stride); /* { dg-error "literal" } */
>> case 7:
>> return vec_gnb (source, 7);
> Terse :-) I think it will work fine though.
Again, I'm sorry this was difficult to review. Originally I thought it would
be easiest
to keep all these together, but that clearly wasn't helpful. I'll work on
breaking
this up.
Thanks for the review!
Bill
>
>
> Segher
