| Message ID | b5708c4a4a07c947a6a969b42b2d2a851af3f49d.camel@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000, Add missing overloaded bcd builtin tests | expand |
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) > {
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) > > {
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
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
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) {
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(-)