Re: [PATCH v7] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]

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

Hi!

On Wed, Nov 24, 2021 at 08:48:47PM -0300, Raoni Fassina Firmino wrote:
> gcc/ChangeLog:
>         * builtins.c (expand_builtin_fegetround): New function.
>         (expand_builtin_feclear_feraise_except): New function.
>         (expand_builtin): Add cases for BUILT_IN_FEGETROUND,
>         BUILT_IN_FECLEAREXCEPT and BUILT_IN_FERAISEEXCEPT

Something is missing here (maybe just a full stop?)

>         * config/rs6000/rs6000.md (fegetroundsi): New pattern.
>         (feclearexceptsi): New Pattern.
>         (feraiseexceptsi): New Pattern.
>         * doc/extend.texi: Add a new introductory paragraph about the
>         new builtins.

Pet peeve: please don't break lines early, we have only 72 columns per
line and we have many long symbol names.  Trying to make many lines very
short only results in everything looking very irregular, which is harder
to read.

>         * doc/md.texi: (fegetround@var{m}): Document new optab.
>         (feclearexcept@var{m}): Document new optab.
>         (feraiseexcept@var{m}): Document new optab.
>         * optabs.def (fegetround_optab): New optab.
>         (feclearexcept_optab): New optab.
>         (feraiseexcept_optab): New optab.
> 
> gcc/testsuite/ChangeLog:
> 
>         * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c: New test.
>         * gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c: New test.
>         * gcc.target/powerpc/builtin-fegetround.c: New test.

> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -6860,6 +6860,117 @@
>    [(set_attr "type" "fpload")
>     (set_attr "length" "8")
>     (set_attr "isa" "*,p8v,p8v")])
> +
> +;; int fegetround(void)
> +;;
> +;; This expansion for the C99 function only expands for compatible
> +;; target libcs. Because it needs to return one of FE_DOWNWARD,
> +;; FE_TONEAREST, FE_TOWARDZERO or FE_UPWARD with the values as defined
> +;; by the target libc, and since they are free to
> +;; choose the values and the expand needs to know then beforehand,
> +;; this expand only expands for target libcs that it can handle the
> +;; values is knows.
> +;; Because of these restriction, this only expands on the desired
> +;; case and fallback to a call to libc on any otherwise.
> +(define_expand "fegetroundsi"

(This needs some wordsmithing.)

> +;; int feclearexcept(int excepts)
> +;;
> +;; This expansion for the C99 function only works when EXCEPTS is a
> +;; constant known at compile time and specifies any one of
> +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> +;; It doesn't handle values out of range, and always returns 0.

It FAILs the expansion if a parameter is bad?  Is this comment out of
date?

> +;; Note that FE_INVALID is unsupported because it maps to more than
> +;; one bit of the FPSCR register.

It could be implemented, now that you check for the libc used.  It is a
fixed part of the ABI :-)

> +;; The FE_* are defined in the targed libc, and since they are free to
> +;; choose the values and the expand needs to know then beforehand,

s/then/them/

> +;; this expand only expands for target libcs that it can handle the

(this expander)

> +;; values is knows.

s/is/it/

> +/* This testcase ensures that the builtins expand with the matching arguments
> + * or otherwise fallback gracefully to a function call, and don't ICE during
> + * compilation.
> + * "-fno-builtin" option is used to enable calls to libc implementation of the
> + * gcc builtins tested when not using __builtin_ prefix. */

Don't use leading * in comments, btw.  This is a testcase so anything
goes, but FYI :-)

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c

> +  int i, rounding, expected;
> +  const int rm[] = {FE_TONEAREST, FE_TOWARDZERO, FE_UPWARD, FE_DOWNWARD};
> +  for (i = 0; i < sizeof(rm); i++)

That should be   sizeof rm / sizeof rm[0]   ?  It accesses out of bounds
as it is.

Maybe test more values?  At least 0, but also combinations of these FE_
bits, and maybe even FE_INVALID?

With such changes the rs6000 parts are okay for trunk.  Thanks!

I looked at the generic changes as well, and they all look fine to me.


Segher