Re: [PATCH] rs6000: Update the vsx-vector-6.* tests.

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Carl,

on 2023/6/22 06:42, Carl Love wrote:
> On Mon, 2023-06-19 at 15:17 +0800, Kewen.Lin wrote:
>> Hi Carl,
>>
>> on 2023/5/31 04:46, Carl Love wrote:
>>> GCC maintainers:
>>>
>>> The following patch takes the tests in vsx-vector-6-p7.h,  vsx-
>>> vector-
>>> 6-p8.h, vsx-vector-6-p9.h and reorganizes them into a series of
>>> smaller
>>> test files by functionality rather than processor version.
>>>
>>> The patch has been tested on Power 10 with no regressions.
>>>
>>> Please let me know if this patch is acceptable for
>>> mainline.  Thanks.
>>>
>>>                        Carl
>>>
>>> ------------------------------------------
>>> rs6000: Update the vsx-vector-6.* tests.
>>>
>>> The vsx-vector-6.h file is included into the processor specific
>>> test files
>>> vsx-vector-6.p7.c, vsx-vector-6.p8.c, and vsx-vector-6.p9.c.  The
>>> .h file
>>> contains a large number of vsx vector builtin tests.  The processor
>>> specific files contain the number of instructions that the tests
>>> are
>>> expected to generate for that processor.  The tests are compile
>>> only.
>>>
>>> The tests are broken up into a seriers of files for related
>>> tests.  The
>>> new tests are runnable tests to verify the builtin argument types
>>> and the
>>
>> But the newly added test cases are all with "dg-do compile", it
>> doesn't
>> match what you said here.
> 
> Ah, yea, that is wrong.  Fixed.
> 
>>
>>> functional correctness of each test rather then verifying the type
>>> and
>>> number of instructions generated.
>>
>> It's good to have more coverage with runnable case, but we miss some
>> test
>> coverages on the expected insn counts which cases p{7,8,9}.c can
>> provide
>> originally.  Unless we can ensure it's already tested somewhere else
>> (do
>> we? it wasn't stated in this patch), I think we still need those
>> checks.
> 
> Yea, I was going with a runnable test and didn't include the
> instruction counts.  Added back in.  Rather then doing by processor
> version (P8, P9, P10) I was able to do it by BE/LE.  The instruction
> counts were the same for LE accross processor versions but there are a
> few instruction counts that vary with BE and LE.

But the original test case only checks for cpu-types (processor version)
but not for endianness, it means for the bif usages, there should not be
different for endianness.  Why does this changes with your new test case?
Could you have a further look and make it consistent with some adjustment
if possible?  As we know, checking insn counts sometimes are fragile, so
I think we should try our best to make it as robust as possible in the
first place.

Besides, the original case also have some differences between p7/p8 and
p9.
  
> 
> I did noticed in one of the tests that the compiler computed the
> answers at compile time and thus didn't actually generate the builtin
> code.  After digging a little more I found a few more tests where the
> compiler was doing the calculations and just inserting the answers.

I'd suggest you also adding function attribute __attribute__((noipa))
to those functions avoiding the possible inlining.

> 
> So, I moved all of the tests to functions so the compiler would
> actually generate the desired builtin code.  
> 
>>
>>> gcc/testsuite/
>>> 	* gcc.target/powerpc/vsx-vector-6-1op.c: New test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-2lop.c: New test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-2op.c: New test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-3op.c: New test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-cmp-all.c: New test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-cmp.c: New test file.
>>> 	* gcc.target/powerpc/vsx-vector-6.h: Remove test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-p7.h: Remove test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-p8.h: Remove test file.
>>> 	* gcc.target/powerpc/vsx-vector-6-p9.h: Remove test file.
>>> ---
>>>  .../powerpc/vsx-vector-6-func-1op.c           | 319 +++++++++++++
>>>  .../powerpc/vsx-vector-6-func-2lop.c          | 305 +++++++++++++
>>>  .../powerpc/vsx-vector-6-func-2op.c           | 278 ++++++++++++
>>>  .../powerpc/vsx-vector-6-func-3op.c           | 229 ++++++++++
>>>  .../powerpc/vsx-vector-6-func-cmp-all.c       | 429
>>> ++++++++++++++++++
>>>  .../powerpc/vsx-vector-6-func-cmp.c           | 237 ++++++++++
>>>  .../gcc.target/powerpc/vsx-vector-6.h         | 154 -------
>>>  .../gcc.target/powerpc/vsx-vector-6.p7.c      |  43 --
>>>  .../gcc.target/powerpc/vsx-vector-6.p8.c      |  43 --
>>>  .../gcc.target/powerpc/vsx-vector-6.p9.c      |  42 --
>>>  10 files changed, 1797 insertions(+), 282 deletions(-)
>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-6-
>>> func-1op.c
>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-6-
>>> func-2lop.c
>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-6-
>>> func-2op.c
>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-6-
>>> func-3op.c
>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-6-
>>> func-cmp-all.c
>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-6-
>>> func-cmp.c
>>>  delete mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-6.h
>>>  delete mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-
>>> 6.p7.c
>>>  delete mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-
>>> 6.p8.c
>>>  delete mode 100644 gcc/testsuite/gcc.target/powerpc/vsx-vector-
>>> 6.p9.c
>>>
>>> diff --git a/gcc/testsuite/gcc.target/powerpc/vsx-vector-6-func-
>>> 1op.c b/gcc/testsuite/gcc.target/powerpc/vsx-vector-6-func-1op.c
>>> new file mode 100644
>>> index 00000000000..90a360ea158
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/powerpc/vsx-vector-6-func-1op.c
>>> @@ -0,0 +1,319 @@
>>> +/* { dg-do compile { target lp64 } } */
>>> +/* { dg-skip-if "" { powerpc*-*-darwin* } } */
>>> +/* { dg-options "-O2 -mdejagnu-cpu=power7" } */
>>> +
>>> +/* Functional test of the one operand vector builtins.  */
>>> +
>>> +#include <altivec.h>
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +
>>> +#define DEBUG 0
>>> +
>>> +void abort (void);
>>> +
>>> +int
>>> +main () {
>>> +  int i;
>>> +  vector float f_src = { 125.44, 23.04, -338.56, 17.64};
>>> +  vector float f_result;
>>> +  vector float f_abs_expected = { 125.44, 23.04, 338.56, 17.64};
>>> +  vector float f_ceil_expected = { 126.0, 24.0, -338, 18.0};
>>> +  vector float f_floor_expected = { 125.0, 23.0, -339, 17.0};
>>> +  vector float f_nearbyint_expected = { 125.0, 23.0, -339, 18.0};
>>> +  vector float f_rint_expected = { 125.0, 23.0, -339, 18.0};
>>> +  vector float f_sqrt_expected = { 11.2, 4.8, 18.4, 4.2};
>>> +  vector float f_trunc_expected = { 125.0, 23.0, -338, 17};
>>> +
>>> +  vector double d_src = { 125.44, -338.56};
>>> +  vector double d_result;
>>> +  vector double d_abs_expected = { 125.44, 338.56};
>>> +  vector double d_ceil_expected = { 126.0, -338.0};
>>> +  vector double d_floor_expected = { 125.0, -339.0};
>>> +  vector double d_nearbyint_expected = { 125.0, -339.0};
>>> +  vector double d_rint_expected = { 125.0, -339.0};
>>> +  vector double d_sqrt_expected = { 11.2, 18.4};
>>> +  vector double d_trunc_expected = { 125.0, -338.0};
>>> +
>>> +  /* Abs, float */
>>> +  f_result = vec_abs (f_src);
>>> +
>>> +  if ((f_result[0] != f_abs_expected[0])
>>> +      || (f_result[1] != f_abs_expected[1])
>>> +      || (f_result[2] != f_abs_expected[2])
>>> +      || (f_result[3] != f_abs_expected[3]))
>>> +#if DEBUG
>>> +    {
>>> +      printf("ERROR: vec_abs (float) expected value does not
>>> match\n");
>>> +      printf("   expected[0] = %f; result[0] = %f\n",
>>> +	     f_abs_expected[0], f_result[0]);
>>> +      printf("   expected[1] = %f; result[1] = %f\n",
>>> +	     f_abs_expected[1], f_result[1]);
>>> +      printf("   expected[2] = %f; result[2] = %f\n",
>>> +	     f_abs_expected[2], f_result[2]);
>>> +      printf("   expected[3] = %f; result[3] = %f\n",
>>> +	     f_abs_expected[3], f_result[3]);
>>> +    }
>>> +#else
>>> +  abort();
>>> +#endif
>>> +
>>> +  /* Ceiling, float */
>>> +  f_result = vec_ceil (f_src);
>>> +
>>> +  if ((f_result[0] != f_ceil_expected[0])
>>> +      || (f_result[1] != f_ceil_expected[1])
>>> +      || (f_result[2] != f_ceil_expected[2])
>>> +      || (f_result[3] != f_ceil_expected[3]))
>>> +#if DEBUG
>>> +    {
>>> +      printf("ERROR: vec_ceil (float) expected value does not
>>> match\n");
>>> +      printf("   expected[0] = %f; result[0] = %f\n",
>>> +	     f_ceil_expected[0], f_result[0]);
>>> +      printf("   expected[1] = %f; result[1] = %f\n",
>>> +	     f_ceil_expected[1], f_result[1]);
>>> +      printf("   expected[2] = %f; result[2] = %f\n",
>>> +	     f_ceil_expected[2], f_result[2]);
>>> +      printf("   expected[3] = %f; result[3] = %f\n",
>>> +	     f_ceil_expected[3], f_result[3]);
>>> +    }
>>> +#else
>>> +  abort();
>>> +#endif
>>
>> It looks that you can use some macro for different floating point
>> functions
>> and its check and dumping here, since the basic skeleton are
>> sharable, some
>> thing like:
>>
>> #define
>> FLOAT_CHECK(NAME)                                                    
>>   \
>>   f_result =
>> vec_##NAME(f_src);                                                \
>>                                                                      
>>           \
>>   if ((f_result[0] != f_##NAME##_expected[0])
>> ||                               \
>>       (f_result[1] != f_##NAME##_expected[1])
>> ||                               \
>>       (f_result[2] != f_##NAME##_expected[2])
>> ||                               \
>>       (f_result[3] != f_##NAME##_expected[3]))
>> {                               \
>>     if (DEBUG)
>> {                                                               \
>>       printf("ERROR: vec_%s (float) expected value does not match\n",
>> #NAME); \
>>       printf("   expected[0] = %f; result[0] = %f\n",
>> f_##NAME##_expected[0],  \
>>              f_result[0]);                                           
>>           \
>>       printf("   expected[1] = %f; result[1] = %f\n",
>> f_##NAME##_expected[1],  \
>>              f_result[1]);                                           
>>           \
>>       printf("   expected[2] = %f; result[2] = %f\n",
>> f_##NAME##_expected[2],  \
>>              f_result[2]);                                           
>>           \
>>       printf("   expected[3] = %f; result[3] = %f\n",
>> f_##NAME##_expected[3],  \
>>              f_result[3]);                                           
>>           \
>>     }
>> else                                                                 
>>     \
>>       abort();                                                       
>>           \
>>   }
>>
>> ...
>> FLOAT_CHECK(ceil)
>> FLOAT_CHECK(nearbyint)
>> ...
>>
>> I hope it can help to make it brief and avoid some copy & paste
>> typos.
>> But since you already expanded them, it's fine to me if you wanted to
>> keep
>> the expanded ones.  :)
> 
> Generally, I am not a big fan of large code macros.  But in this case
> it is probably a good idea given the repetitive code.  When I wrote the
> test cases, I had to put comments in to identify were each test started
> as it gets lost in the code.  Use of a macro makes it much easier to
> see what cases are being tested.

Yeah, it should help to avoid those typos.  Thanks for updating.

BR,
Kewen