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

[Segher Boessenkool <segher@kernel.crashing.org>]

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.

>  - 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.

>  - 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)?


> --- 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.

> --- 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).

> --- 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?

> --- 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.

> --- 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.

> --- 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 :-(

> --- 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?

> --- 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.

> --- 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.


Segher