rs6000, Add missing overloaded bcd builtin tests

Message ID b5708c4a4a07c947a6a969b42b2d2a851af3f49d.camel@linux.ibm.com
State Accepted
Headers
Series rs6000, Add missing overloaded bcd builtin tests |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Carl Love Oct. 31, 2023, 12:08 a.m. UTC
  GCC maintainers:

The following patch adds tests for two of the rs6000 overloaded built-
ins that do not have tests.  Additionally the GCC documentation file
doc/extend.texi is updated to include the built-in definitions as they
were missing.

The patch has been tested on a Power 10 system with no regressions. 
Please let me know if this patch is acceptable for mainline.

                 Carl

-------------------------------------------
rs6000, Add missing overloaded bcd builtin tests

The two BCD overloaded built-ins __builtin_bcdsub_ge and __builtin_bcdsub_le
do not have a corresponding test.  Add tests to existing test file and update
the documentation with the built-in definitions.

gcc/ChangeLog:
	* doc/extend.texi (__builtin_bcdsub_le, __builtin_bcdsub_ge): Add
	documentation for the builti-ins.

gcc/testsuite/ChangeLog:
	* bcd-3.c (do_sub_ge, do_suble): Add functions to test builtins
	__builtin_bcdsub_ge and __builtin_bcdsub_le).
---
 gcc/doc/extend.texi                      |  4 ++++
 gcc/testsuite/gcc.target/powerpc/bcd-3.c | 22 +++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)
  

Comments

Kewen.Lin Oct. 31, 2023, 2:34 a.m. UTC | #1
Hi Carl,

on 2023/10/31 08:08, Carl Love wrote:
> GCC maintainers:
> 
> The following patch adds tests for two of the rs6000 overloaded built-
> ins that do not have tests.  Additionally the GCC documentation file

I just found that actually they have the test coverage, because we have

#define __builtin_bcdcmpeq(a,b)   __builtin_vec_bcdsub_eq(a,b,0)
#define __builtin_bcdcmpgt(a,b)   __builtin_vec_bcdsub_gt(a,b,0)
#define __builtin_bcdcmplt(a,b)   __builtin_vec_bcdsub_lt(a,b,0)
#define __builtin_bcdcmpge(a,b)   __builtin_vec_bcdsub_ge(a,b,0)
#define __builtin_bcdcmple(a,b)   __builtin_vec_bcdsub_le(a,b,0)

in altivec.h and gcc/testsuite/gcc.target/powerpc/bcd-4.c tests all these
__builtin_bcdcmp* ...

> doc/extend.texi is updated to include the built-in definitions as they
> were missing.

... since we already document __builtin_vec_bcdsub_{eq,gt,lt}, I think
it's still good to supplement the documentation and add the explicit
testing cases.

> 
> The patch has been tested on a Power 10 system with no regressions. 
> Please let me know if this patch is acceptable for mainline.
> 
>                  Carl
> 
> -------------------------------------------
> rs6000, Add missing overloaded bcd builtin tests
> 
> The two BCD overloaded built-ins __builtin_bcdsub_ge and __builtin_bcdsub_le
> do not have a corresponding test.  Add tests to existing test file and update
> the documentation with the built-in definitions.

As above, this commit log doesn't describe the actuality well, please update
it with something like:

Currently we have the documentation for __builtin_vec_bcdsub_{eq,gt,lt} but
not for __builtin_bcdsub_[gl]e, this patch is to supplement the descriptions
for them.  Although they are mainly for __builtin_bcdcmp{ge,le}, we already
have some testing coverage for __builtin_vec_bcdsub_{eq,gt,lt}, this patch
adds the corresponding explicit test cases as well.

> 
> gcc/ChangeLog:
> 	* doc/extend.texi (__builtin_bcdsub_le, __builtin_bcdsub_ge): Add
> 	documentation for the builti-ins.
> 
> gcc/testsuite/ChangeLog:
> 	* bcd-3.c (do_sub_ge, do_suble): Add functions to test builtins
> 	__builtin_bcdsub_ge and __builtin_bcdsub_le).

1) Unexpected ")" at the end.

2) I supposed git gcc-verify would complain on this changelog entry.

Should be starting with:

	* gcc.target/powerpc/bcd-3.c (....

, no?

OK for trunk with the above comments addressed, thanks!

BR,
Kewen

> ---
>  gcc/doc/extend.texi                      |  4 ++++
>  gcc/testsuite/gcc.target/powerpc/bcd-3.c | 22 +++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index cf0d0c63cce..fa7402813e7 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -20205,12 +20205,16 @@ int __builtin_bcdadd_ov (vector unsigned char, vector unsigned char, const int);
>  vector __int128 __builtin_bcdsub (vector __int128, vector __int128, const int);
>  vector unsigned char __builtin_bcdsub (vector unsigned char, vector unsigned char,
>                                         const int);
> +int __builtin_bcdsub_le (vector __int128, vector __int128, const int);
> +int __builtin_bcdsub_le (vector unsigned char, vector unsigned char, const int);
>  int __builtin_bcdsub_lt (vector __int128, vector __int128, const int);
>  int __builtin_bcdsub_lt (vector unsigned char, vector unsigned char, const int);
>  int __builtin_bcdsub_eq (vector __int128, vector __int128, const int);
>  int __builtin_bcdsub_eq (vector unsigned char, vector unsigned char, const int);
>  int __builtin_bcdsub_gt (vector __int128, vector __int128, const int);
>  int __builtin_bcdsub_gt (vector unsigned char, vector unsigned char, const int);
> +int __builtin_bcdsub_ge (vector __int128, vector __int128, const int);
> +int __builtin_bcdsub_ge (vector unsigned char, vector unsigned char, const int);
>  int __builtin_bcdsub_ov (vector __int128, vector __int128, const int);
>  int __builtin_bcdsub_ov (vector unsigned char, vector unsigned char, const int);
>  @end smallexample
> diff --git a/gcc/testsuite/gcc.target/powerpc/bcd-3.c b/gcc/testsuite/gcc.target/powerpc/bcd-3.c
> index 7948a0c95e2..9891f4ff08e 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bcd-3.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bcd-3.c
> @@ -3,7 +3,7 @@
>  /* { dg-require-effective-target powerpc_p8vector_ok } */
>  /* { dg-options "-mdejagnu-cpu=power8 -O2" } */
>  /* { dg-final { scan-assembler-times "bcdadd\[.\] " 4 } } */
> -/* { dg-final { scan-assembler-times "bcdsub\[.\] " 4 } } */
> +/* { dg-final { scan-assembler-times "bcdsub\[.\] " 6 } } */
>  /* { dg-final { scan-assembler-not   "bl __builtin"   } } */
>  /* { dg-final { scan-assembler-not   "mtvsr"   	      } } */
>  /* { dg-final { scan-assembler-not   "mfvsr"   	      } } */
> @@ -93,6 +93,26 @@ do_sub_gt (vector_128_t a, vector_128_t b, int *p)
>    return ret;
>  }
>  
> +vector_128_t
> +do_sub_ge (vector_128_t a, vector_128_t b, int *p)
> +{
> +  vector_128_t ret = __builtin_bcdsub (a, b, 0);
> +  if (__builtin_bcdsub_ge (a, b, 0))
> +    *p = 1;
> +
> +  return ret;
> +}
> +
> +vector_128_t
> +do_sub_le (vector_128_t a, vector_128_t b, int *p)
> +{
> +  vector_128_t ret = __builtin_bcdsub (a, b, 0);
> +  if (__builtin_bcdsub_le (a, b, 0))
> +    *p = 1;
> +
> +  return ret;
> +}
> +
>  vector_128_t
>  do_sub_ov (vector_128_t a, vector_128_t b, int *p)
>  {
  
Carl Love Oct. 31, 2023, 3:31 p.m. UTC | #2
On Tue, 2023-10-31 at 10:34 +0800, Kewen.Lin wrote:
> Hi Carl,
> 
> on 2023/10/31 08:08, Carl Love wrote:
> > GCC maintainers:
> > 
> > The following patch adds tests for two of the rs6000 overloaded
> > built-
> > ins that do not have tests.  Additionally the GCC documentation
> > file
> 
> I just found that actually they have the test coverage, because we
> have
> 
> #define __builtin_bcdcmpeq(a,b)   __builtin_vec_bcdsub_eq(a,b,0)
> #define __builtin_bcdcmpgt(a,b)   __builtin_vec_bcdsub_gt(a,b,0)
> #define __builtin_bcdcmplt(a,b)   __builtin_vec_bcdsub_lt(a,b,0)
> #define __builtin_bcdcmpge(a,b)   __builtin_vec_bcdsub_ge(a,b,0)
> #define __builtin_bcdcmple(a,b)   __builtin_vec_bcdsub_le(a,b,0)
> 
> in altivec.h and gcc/testsuite/gcc.target/powerpc/bcd-4.c tests all
> these

OK, my simple scripts are not going to pickup the stuff in altivec.h. 
They were just grepping for the built-in name in the test file
directory.

> __builtin_bcdcmp* ...
> 
> > doc/extend.texi is updated to include the built-in definitions as
> > they
> > were missing.
> 
> ... since we already document __builtin_vec_bcdsub_{eq,gt,lt}, I
> think
> it's still good to supplement the documentation and add the explicit
> testing cases.
> 
> > The patch has been tested on a Power 10 system with no
> > regressions. 
> > Please let me know if this patch is acceptable for mainline.
> > 
> >                  Carl
> > 
> > -------------------------------------------
> > rs6000, Add missing overloaded bcd builtin tests
> > 
> > The two BCD overloaded built-ins __builtin_bcdsub_ge and
> > __builtin_bcdsub_le
> > do not have a corresponding test.  Add tests to existing test file
> > and update
> > the documentation with the built-in definitions.
> 
> As above, this commit log doesn't describe the actuality well, please
> update
> it with something like:
> 
> Currently we have the documentation for
> __builtin_vec_bcdsub_{eq,gt,lt} but
> not for __builtin_bcdsub_[gl]e, this patch is to supplement the
> descriptions
> for them.  Although they are mainly for __builtin_bcdcmp{ge,le}, we
> already
> have some testing coverage for __builtin_vec_bcdsub_{eq,gt,lt}, this
> patch
> adds the corresponding explicit test cases as well.
> 

OK, replaced the commit log with the suggestion.

> > gcc/ChangeLog:
> > 	* doc/extend.texi (__builtin_bcdsub_le, __builtin_bcdsub_ge):
> > Add
> > 	documentation for the builti-ins.
> > 
> > gcc/testsuite/ChangeLog:
> > 	* bcd-3.c (do_sub_ge, do_suble): Add functions to test builtins
> > 	__builtin_bcdsub_ge and __builtin_bcdsub_le).
> 
> 1) Unexpected ")" at the end.
> 
> 2) I supposed git gcc-verify would complain on this changelog entry.
> 
> Should be starting with:
> 
> 	* gcc.target/powerpc/bcd-3.c (....
> 
> , no?
> 

Yes, I ment to run the commit check but obviously got distracted and
didn't.  Sorry about that.  

> OK for trunk with the above comments addressed, thanks!
> 
OK, thanks.

                    Carl 

> BR,
> Kewen
> 
> > ---
> >  gcc/doc/extend.texi                      |  4 ++++
> >  gcc/testsuite/gcc.target/powerpc/bcd-3.c | 22
> > +++++++++++++++++++++-
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > index cf0d0c63cce..fa7402813e7 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -20205,12 +20205,16 @@ int __builtin_bcdadd_ov (vector unsigned
> > char, vector unsigned char, const int);
> >  vector __int128 __builtin_bcdsub (vector __int128, vector
> > __int128, const int);
> >  vector unsigned char __builtin_bcdsub (vector unsigned char,
> > vector unsigned char,
> >                                         const int);
> > +int __builtin_bcdsub_le (vector __int128, vector __int128, const
> > int);
> > +int __builtin_bcdsub_le (vector unsigned char, vector unsigned
> > char, const int);
> >  int __builtin_bcdsub_lt (vector __int128, vector __int128, const
> > int);
> >  int __builtin_bcdsub_lt (vector unsigned char, vector unsigned
> > char, const int);
> >  int __builtin_bcdsub_eq (vector __int128, vector __int128, const
> > int);
> >  int __builtin_bcdsub_eq (vector unsigned char, vector unsigned
> > char, const int);
> >  int __builtin_bcdsub_gt (vector __int128, vector __int128, const
> > int);
> >  int __builtin_bcdsub_gt (vector unsigned char, vector unsigned
> > char, const int);
> > +int __builtin_bcdsub_ge (vector __int128, vector __int128, const
> > int);
> > +int __builtin_bcdsub_ge (vector unsigned char, vector unsigned
> > char, const int);
> >  int __builtin_bcdsub_ov (vector __int128, vector __int128, const
> > int);
> >  int __builtin_bcdsub_ov (vector unsigned char, vector unsigned
> > char, const int);
> >  @end smallexample
> > diff --git a/gcc/testsuite/gcc.target/powerpc/bcd-3.c
> > b/gcc/testsuite/gcc.target/powerpc/bcd-3.c
> > index 7948a0c95e2..9891f4ff08e 100644
> > --- a/gcc/testsuite/gcc.target/powerpc/bcd-3.c
> > +++ b/gcc/testsuite/gcc.target/powerpc/bcd-3.c
> > @@ -3,7 +3,7 @@
> >  /* { dg-require-effective-target powerpc_p8vector_ok } */
> >  /* { dg-options "-mdejagnu-cpu=power8 -O2" } */
> >  /* { dg-final { scan-assembler-times "bcdadd\[.\] " 4 } } */
> > -/* { dg-final { scan-assembler-times "bcdsub\[.\] " 4 } } */
> > +/* { dg-final { scan-assembler-times "bcdsub\[.\] " 6 } } */
> >  /* { dg-final { scan-assembler-not   "bl __builtin"   } } */
> >  /* { dg-final { scan-assembler-not   "mtvsr"   	      } } */
> >  /* { dg-final { scan-assembler-not   "mfvsr"   	      } } */
> > @@ -93,6 +93,26 @@ do_sub_gt (vector_128_t a, vector_128_t b, int
> > *p)
> >    return ret;
> >  }
> >  
> > +vector_128_t
> > +do_sub_ge (vector_128_t a, vector_128_t b, int *p)
> > +{
> > +  vector_128_t ret = __builtin_bcdsub (a, b, 0);
> > +  if (__builtin_bcdsub_ge (a, b, 0))
> > +    *p = 1;
> > +
> > +  return ret;
> > +}
> > +
> > +vector_128_t
> > +do_sub_le (vector_128_t a, vector_128_t b, int *p)
> > +{
> > +  vector_128_t ret = __builtin_bcdsub (a, b, 0);
> > +  if (__builtin_bcdsub_le (a, b, 0))
> > +    *p = 1;
> > +
> > +  return ret;
> > +}
> > +
> >  vector_128_t
> >  do_sub_ov (vector_128_t a, vector_128_t b, int *p)
> >  {
  
Segher Boessenkool Oct. 31, 2023, 4:17 p.m. UTC | #3
On Tue, Oct 31, 2023 at 08:31:25AM -0700, Carl Love wrote:
> > I just found that actually they have the test coverage, because we
> > have
> > 
> > #define __builtin_bcdcmpeq(a,b)   __builtin_vec_bcdsub_eq(a,b,0)
> > #define __builtin_bcdcmpgt(a,b)   __builtin_vec_bcdsub_gt(a,b,0)
> > #define __builtin_bcdcmplt(a,b)   __builtin_vec_bcdsub_lt(a,b,0)
> > #define __builtin_bcdcmpge(a,b)   __builtin_vec_bcdsub_ge(a,b,0)
> > #define __builtin_bcdcmple(a,b)   __builtin_vec_bcdsub_le(a,b,0)
> > 
> > in altivec.h and gcc/testsuite/gcc.target/powerpc/bcd-4.c tests all
> > these
> 
> OK, my simple scripts are not going to pickup the stuff in altivec.h. 
> They were just grepping for the built-in name in the test file
> directory.

You could use gcov to see which rs6000 builtins are not exercised by
anything in the testsuite, maybe.  This probably can be automated pretty
nicely.


Segher
  
Carl Love Oct. 31, 2023, 5:27 p.m. UTC | #4
Segher:

On Tue, 2023-10-31 at 11:17 -0500, Segher Boessenkool wrote:

<snip>
> 
> You could use gcov to see which rs6000 builtins are not exercised by
> anything in the testsuite, maybe.  This probably can be automated
> pretty
> nicely.

I will take a look at gcov.  I just did some relatively simple scripts
to go look for test cases.  For the non-overloaded built-ins, the
scrips had to exclude built-ins referenced by the overloaded built-ins.

This patch is just the first of a series of patches that I am working
on to try and clean up the built-in stuff per some comments in a PR. 
The internal LTC issue is
 
https://github.ibm.com/ltc-toolchain/power-gcc/issues/1288

The goal is to make sure there are test cases and documentation for all
of the overloaded and non overloaded built-in definitions.  Just a low
priority project to fill any spare cycles.  :-)

                      Carl
  

Patch

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index cf0d0c63cce..fa7402813e7 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -20205,12 +20205,16 @@  int __builtin_bcdadd_ov (vector unsigned char, vector unsigned char, const int);
 vector __int128 __builtin_bcdsub (vector __int128, vector __int128, const int);
 vector unsigned char __builtin_bcdsub (vector unsigned char, vector unsigned char,
                                        const int);
+int __builtin_bcdsub_le (vector __int128, vector __int128, const int);
+int __builtin_bcdsub_le (vector unsigned char, vector unsigned char, const int);
 int __builtin_bcdsub_lt (vector __int128, vector __int128, const int);
 int __builtin_bcdsub_lt (vector unsigned char, vector unsigned char, const int);
 int __builtin_bcdsub_eq (vector __int128, vector __int128, const int);
 int __builtin_bcdsub_eq (vector unsigned char, vector unsigned char, const int);
 int __builtin_bcdsub_gt (vector __int128, vector __int128, const int);
 int __builtin_bcdsub_gt (vector unsigned char, vector unsigned char, const int);
+int __builtin_bcdsub_ge (vector __int128, vector __int128, const int);
+int __builtin_bcdsub_ge (vector unsigned char, vector unsigned char, const int);
 int __builtin_bcdsub_ov (vector __int128, vector __int128, const int);
 int __builtin_bcdsub_ov (vector unsigned char, vector unsigned char, const int);
 @end smallexample
diff --git a/gcc/testsuite/gcc.target/powerpc/bcd-3.c b/gcc/testsuite/gcc.target/powerpc/bcd-3.c
index 7948a0c95e2..9891f4ff08e 100644
--- a/gcc/testsuite/gcc.target/powerpc/bcd-3.c
+++ b/gcc/testsuite/gcc.target/powerpc/bcd-3.c
@@ -3,7 +3,7 @@ 
 /* { dg-require-effective-target powerpc_p8vector_ok } */
 /* { dg-options "-mdejagnu-cpu=power8 -O2" } */
 /* { dg-final { scan-assembler-times "bcdadd\[.\] " 4 } } */
-/* { dg-final { scan-assembler-times "bcdsub\[.\] " 4 } } */
+/* { dg-final { scan-assembler-times "bcdsub\[.\] " 6 } } */
 /* { dg-final { scan-assembler-not   "bl __builtin"   } } */
 /* { dg-final { scan-assembler-not   "mtvsr"   	      } } */
 /* { dg-final { scan-assembler-not   "mfvsr"   	      } } */
@@ -93,6 +93,26 @@  do_sub_gt (vector_128_t a, vector_128_t b, int *p)
   return ret;
 }
 
+vector_128_t
+do_sub_ge (vector_128_t a, vector_128_t b, int *p)
+{
+  vector_128_t ret = __builtin_bcdsub (a, b, 0);
+  if (__builtin_bcdsub_ge (a, b, 0))
+    *p = 1;
+
+  return ret;
+}
+
+vector_128_t
+do_sub_le (vector_128_t a, vector_128_t b, int *p)
+{
+  vector_128_t ret = __builtin_bcdsub (a, b, 0);
+  if (__builtin_bcdsub_le (a, b, 0))
+    *p = 1;
+
+  return ret;
+}
+
 vector_128_t
 do_sub_ov (vector_128_t a, vector_128_t b, int *p)
 {