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