[PATCH v2] rs6000: Test case adjustments for new builtins

Bill Schmidt wschmidt@linux.ibm.com
Wed Nov 17 13:52:38 GMT 2021


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


More information about the Gcc-patches mailing list