Re: [PATCH] rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Hi!
On Fri, Sep 04, 2020 at 12:44:17PM -0300, Raoni Fassina Firmino wrote:
> > > + switch (INTVAL (operands[1]))
> > > +{
> > > +case (1 << (31 - 6)): /* FE_INEXACT */
> >
> > I would just write it as 0x02000 etc.? much clearer, and you have
> > the comment demagicificating it anyway!
>
> At first I used the plain hex numbers, but I worried that it was easy to
> get it wrong (too many zeroes)
That's why you always use exactly 8 digits (which I got wrong, not
making a very strong case there, lol).
The "31 -" always is annoying noise, and (1 << 31) is *negative* (or
undefined if you want), so you need 1U (or 1ULL) really, making it even
less readable.
So pick your poison, there are no great solutions :-/
> and thought that copying verbatim from
> glibc was the way to go.
That would be nice, certainly.
> > > +OPTAB_D (fegetround_optab, "fegetround$a")
> > > +OPTAB_D (feclearexcept_optab, "feclearexcept$a")
> > > +OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
> >
> > Should those be documented somewhere? (In gcc/doc/ somewhere).
>
> I am lost on that one. I took a look on the docs (I hope looking on the
> online docs was good enough)
If you looked at the internals documentation that has exactly all
content, yes.
https://gcc.gnu.org/onlinedocs/gccint/
https://gcc.gnu.org/onlinedocs/gccint.pdf
(but it sounds like you looked at the user manual instead?)
Many such things are documented in md.texi .
> > > --- /dev/null
> > > +++
> > > b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
> > > @@ -0,0 +1,64 @@
> > > +/* { dg-do run { target { powerpc*-*-* } } } */
> >
> > All files in gcc.target/powerpc/ are run for powerpc already; just
> > /* { dg-do run } */
> > please.
>
> Another case of me copying-around. Done.
There are so many bad (or dated) examples in existing tests :-(
> > > +/* { dg-options "-lm -fno-builtin" } */
> >
> > Does that work everywhere? AIX, Darwin, other non-Linux systems, systems
> > without OS, etc.
>
> You mean because I am linking the tests with libm right, not the
> -fno-builtin? I guess, in principle we don't need a libc to test the
> builtins, but I thought I would ended up reimplementing the builtins
> themselves and other fenv.h functions (with chances of getting them
> wrong), and that doing the test comparing against the standard library
> implementation would be more robust. Should I change it?
I don't know, that's why I asked :-) We'll just have to try it out.
Thanks,
Segher
Re: [PATCH] rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Hi,
I am about to sent a v2, but thought to reply here as well.
On Tue, Aug 18, 2020 at 07:09:21PM -0500, Segher Boessenkool wrote:
> Hi!
>
> On Fri, Aug 14, 2020 at 07:54:23PM -0300, Raoni Fassina Firmino via
> Gcc-patches wrote:
> > So, this patch adds new rs6000 expand optimizations for fegetround and
> > for some calls to feclearexcept and feraiseexcept. All of them C99
> > functions from fenv.h
>
> And the fenv.h implementation can then use the builtins.
>
> > To check the FE_* flags used in feclearexcept and feraiseexcept
> > expands I decided copy verbatim the definitions from glibc instead of
> > using the macros, which would means including fenv.h somewhere to get
> > them.
>
> Good plan :-)
>
> > Still on feclearexcept and feraiseexcept I, I am not sure I used
> > exact_log2_cint_operand correctly because on my tests it kept
> > accepting feclearexcept(0) and it should not.
>
> I am not sure what you mean. If you pass a number not one of the four
> allowed ones, the pattern FAILs anyway?
It should fail but it was letting 0 pass and them the rule would
segfault/panic when trying to manipulate the input. But as I said, I
think I was using it wrong, change a lot of things at the same time,
including the test code.
>
> In fact, you could just use const_int_operand with "n"?
Yes, it seems better. Done.
>
> > In any case, because I
> > decided to test for all valid flags, this is not a problem for correct
> > generation, but I thought I should mention it.
>
> Ah gotcha. Yes, see above.
>
> > --- a/gcc/builtins.c
> > +++ b/gcc/builtins.c
> > @@ -115,6 +115,8 @@ static rtx expand_builtin_mathfn_3 (tree, rtx, rtx);
> > static rtx expand_builtin_mathfn_ternary (tree, rtx, rtx);
> > static rtx expand_builtin_interclass_mathfn (tree, rtx);
> > static rtx expand_builtin_sincos (tree);
> > +static rtx expand_builtin_fegetround (tree, rtx, machine_mode);
> > +static rtx expand_builtin_feclear_feraise_except (tree, rtx, machine_mode,
> > optab);
>
> That last line is too long, please break it?
Done.
>
> > +/* Expand call EXP to the fegetround builtin (from C99 venv.h), returning
> > the
>
> "fenv.h"
Done.
>
> > + if (target == 0
> > + || GET_MODE (target) != target_mode
> > + || ! (*insn_data[icode].operand[0].predicate) (target, target_mode))
> > +target = gen_reg_rtx (target_mode);
> > +
> > + rtx pat = GEN_FCN (icode) (target);
> > + if (! pat)
> > +return NULL_RTX;
> > + emit_insn (pat);
>
> No space after unary operators (like !) please (the exception is those
> written with alphabetics, like casts and sizeof).
Done.
>
> I guess you copied this, so I don't know -- have to stop bad habits
> somewhere I guess :-)
That was exactly it. I use this shortcut of trying to copy-and-modify
from code around to keep the style consistent but sometimes, like here,
it backfires when the original code did not follow the standard.
>
> > +;; int __builtin_fegetround()
> > +(define_expand "fegetroundsi"
> > + [(use (match_operand:SI 0 "gpc_reg_operand"))]
> > + "TARGET_HARD_FLOAT"
> > +{
> > +rtx tmp_df = gen_reg_rtx (DFmode);
>
> Indentation should be just two spaces.
Done.
>
> > +;; int feclearexcept(int excepts)
> > +;;
> > +;; This expansion for the C99 function only works when excepts is a
> > +;; constant know at compile time and specifying only one of
> > +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> > +;; It dosen't handle values out of range, and always returns 0.
>
> (doesn't)
Done.
>
> > +;; Note that FE_INVALID is unsuported because it maps to more than
>
> (unsupported)
Done.
>
> > +;; one bit on FPSCR register.
>
> You cannot set or clear the VX bit directly, yes (you have to twiddle
> the component VX* bits you care about). Which we could do later
> perhaps, but this is fine now :-)
:)
>
> > +;; Because this restrictions, this only expands on the desired cases.
>
> (Because of these)
Done.
>
> > +(define_expand "feclearexceptsi"
> > + [(use (match_operand:SI 1 "exact_log2_cint_operand" "N"))
>
> So just "const_int_operand" "n" should work fine here, and make it
> more obvious that it won't actually allow all numbers.
Done.
>
> > + switch (INTVAL (operands[1]))
> > +{
> > +case (1 << (31 - 6)): /* FE_INEXACT */
>
> I would just write it as 0x02000 etc.? much clearer, and you have
> the comment demagicificating it anyway!
At first I used the plain hex numbers, but I worried that it was easy to
get it wrong (too many zeroes) and thought that copying verbatim from
glibc was the way to go. But with the tests it is not really a concern I
guess. I have not strong opinion one way or another, I will change in v2.
Done.
>
> > +case (1 << (31 - 5)): /* FE_DIVBYZERO */
> > +case (1 << (31 - 4)): /* FE_UNDERFLOW */
> > +case (1 << (31 - 3)): /* FE_OVERFLOW */
> > + break;
> > +default:
> > + FAIL;
> > +}
> > +
> > + rtx tmp = gen_rtx_CONST_INT (SImode,
Re: [PATCH] rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Hi!
On Fri, Aug 14, 2020 at 07:54:23PM -0300, Raoni Fassina Firmino via Gcc-patches
wrote:
> So, this patch adds new rs6000 expand optimizations for fegetround and
> for some calls to feclearexcept and feraiseexcept. All of them C99
> functions from fenv.h
And the fenv.h implementation can then use the builtins.
> To check the FE_* flags used in feclearexcept and feraiseexcept
> expands I decided copy verbatim the definitions from glibc instead of
> using the macros, which would means including fenv.h somewhere to get
> them.
Good plan :-)
> Still on feclearexcept and feraiseexcept I, I am not sure I used
> exact_log2_cint_operand correctly because on my tests it kept
> accepting feclearexcept(0) and it should not.
I am not sure what you mean. If you pass a number not one of the four
allowed ones, the pattern FAILs anyway?
In fact, you could just use const_int_operand with "n"?
> In any case, because I
> decided to test for all valid flags, this is not a problem for correct
> generation, but I thought I should mention it.
Ah gotcha. Yes, see above.
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -115,6 +115,8 @@ static rtx expand_builtin_mathfn_3 (tree, rtx, rtx);
> static rtx expand_builtin_mathfn_ternary (tree, rtx, rtx);
> static rtx expand_builtin_interclass_mathfn (tree, rtx);
> static rtx expand_builtin_sincos (tree);
> +static rtx expand_builtin_fegetround (tree, rtx, machine_mode);
> +static rtx expand_builtin_feclear_feraise_except (tree, rtx, machine_mode,
> optab);
That last line is too long, please break it?
> +/* Expand call EXP to the fegetround builtin (from C99 venv.h), returning the
"fenv.h"
> + if (target == 0
> + || GET_MODE (target) != target_mode
> + || ! (*insn_data[icode].operand[0].predicate) (target, target_mode))
> +target = gen_reg_rtx (target_mode);
> +
> + rtx pat = GEN_FCN (icode) (target);
> + if (! pat)
> +return NULL_RTX;
> + emit_insn (pat);
No space after unary operators (like !) please (the exception is those
written with alphabetics, like casts and sizeof).
I guess you copied this, so I don't know -- have to stop bad habits
somewhere I guess :-)
> +;; int __builtin_fegetround()
> +(define_expand "fegetroundsi"
> + [(use (match_operand:SI 0 "gpc_reg_operand"))]
> + "TARGET_HARD_FLOAT"
> +{
> +rtx tmp_df = gen_reg_rtx (DFmode);
Indentation should be just two spaces.
> +;; int feclearexcept(int excepts)
> +;;
> +;; This expansion for the C99 function only works when excepts is a
> +;; constant know at compile time and specifying only one of
> +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> +;; It dosen't handle values out of range, and always returns 0.
(doesn't)
> +;; Note that FE_INVALID is unsuported because it maps to more than
(unsupported)
> +;; one bit on FPSCR register.
You cannot set or clear the VX bit directly, yes (you have to twiddle
the component VX* bits you care about). Which we could do later
perhaps, but this is fine now :-)
> +;; Because this restrictions, this only expands on the desired cases.
(Because of these)
> +(define_expand "feclearexceptsi"
> + [(use (match_operand:SI 1 "exact_log2_cint_operand" "N"))
So just "const_int_operand" "n" should work fine here, and make it
more obvious that it won't actually allow all numbers.
> + switch (INTVAL (operands[1]))
> +{
> +case (1 << (31 - 6)): /* FE_INEXACT */
I would just write it as 0x02000 etc.? much clearer, and you have
the comment demagicificating it anyway!
> +case (1 << (31 - 5)): /* FE_DIVBYZERO */
> +case (1 << (31 - 4)): /* FE_UNDERFLOW */
> +case (1 << (31 - 3)): /* FE_OVERFLOW */
> + break;
> +default:
> + FAIL;
> +}
> +
> + rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL(operands[1])));
Space after "INTVAL".
> + emit_insn (gen_rs6000_mtfsb0 (tmp));
> + emit_move_insn (operands[0], GEN_INT (0));
> + DONE;
> +})
GEN_INT (0) is just const0_rtx , please use that?
> +(define_expand "feraiseexceptsi"
> + [(use (match_operand:SI 1 "exact_log2_cint_operand" "N"))
> + (set (match_operand:SI 0 "gpc_reg_operand")
> +(const_int 0))]
Indent by 8 spaces should be a tab (here and elsewhere).
> +OPTAB_D (fegetround_optab, "fegetround$a")
> +OPTAB_D (feclearexcept_optab, "feclearexcept$a")
> +OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
Should those be documented somewhere? (In gcc/doc/ somewhere).
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
> @@ -0,0 +1,64 @@
> +/* { dg-do run { target { powerpc*-*-* } } } */
All files in gcc.target/powerpc/ are run for powerpc already; just
/* { dg-do run } */
please.
> +/* { dg-options "-lm -fno-builtin" } */
Does that work everywhere? AIX, Darwin, other non-Linux systems, systems
without OS, etc.
> +#include
That header does not exist everywhere. You can just declare the things
you need (the FE_ constants?)
Or perhaps you want to
[PATCH] rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Hi All,
I sent an initial RFC of this patch[1] some time ago. I believe it is
now in a complete state, with added the test cases for the builtins.
The builtin optimizations presented here were originally in glibc, but
were removed and suggested that they were a good fit as gcc
builtins[2].
So, this patch adds new rs6000 expand optimizations for fegetround and
for some calls to feclearexcept and feraiseexcept. All of them C99
functions from fenv.h
I replicated the optimizations semantics almost as-is as the glibc
version, with the notable exception that for feclearexcept and
feraiseexcept, the glibc builtin was not filtering only the valid
flags, so it had and undefined behavior for values out of range. For
the gcc builtin I thought was best to explicitly filter the valid ones,
as the builtin does not return any error(same as the original) and
there is only 4 valid flags.
To check the FE_* flags used in feclearexcept and feraiseexcept
expands I decided copy verbatim the definitions from glibc instead of
using the macros, which would means including fenv.h somewhere to get
them.
Still on feclearexcept and feraiseexcept I, I am not sure I used
exact_log2_cint_operand correctly because on my tests it kept
accepting feclearexcept(0) and it should not. In any case, because I
decided to test for all valid flags, this is not a problem for correct
generation, but I thought I should mention it.
tested on top of master (808f4dfeb3a95f50f15e71148e5c1067f90a126d)
on the following plataforms with no regression:
powerpc64le-linux-gnu (Power 9)
powerpc64le-linux-gnu (Power 8)
[1] https://gcc.gnu.org/pipermail/gcc-patches/2020-June/548998.html
[2] https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00047.html
https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00080.html
8<
This optimizations were originally in glibc, but was removed
and sugested that they were a good fit as gcc builtins[1].
The associated bugreport: PR target/94193
[1] https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00047.html
https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00080.html
2020-08-13 Raoni Fassina Firmino
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
* config/rs6000/rs6000.md (fegetroundsi): New pattern.
(feclearexceptsi): New Pattern.
(feraiseexceptsi): New Pattern.
* 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.
Signed-off-by: Raoni Fassina Firmino
---
gcc/builtins.c| 75 ++
gcc/config/rs6000/rs6000.md | 82 +++
gcc/optabs.def| 4 +
.../builtin-feclearexcept-feraiseexcept-1.c | 64 +
.../builtin-feclearexcept-feraiseexcept-2.c | 130 ++
.../gcc.target/powerpc/builtin-fegetround.c | 30
6 files changed, 385 insertions(+)
create mode 100644
gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
create mode 100644
gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c
create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c
diff --git a/gcc/builtins.c b/gcc/builtins.c
index beb56e06d8a..de6f34e0225 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -115,6 +115,8 @@ static rtx expand_builtin_mathfn_3 (tree, rtx, rtx);
static rtx expand_builtin_mathfn_ternary (tree, rtx, rtx);
static rtx expand_builtin_interclass_mathfn (tree, rtx);
static rtx expand_builtin_sincos (tree);
+static rtx expand_builtin_fegetround (tree, rtx, machine_mode);
+static rtx expand_builtin_feclear_feraise_except (tree, rtx, machine_mode,
optab);
static rtx expand_builtin_cexpi (tree, rtx);
static rtx expand_builtin_int_roundingfn (tree, rtx);
static rtx expand_builtin_int_roundingfn_2 (tree, rtx);
@@ -2577,6 +2579,59 @@ expand_builtin_sincos (tree exp)
return const0_rtx;
}
+/* Expand call EXP to the fegetround builtin (from C99 venv.h), returning the
+ result and setting it in TARGET. Otherwise return NULL_RTX on failure. */
+static rtx
+expand_builtin_fegetround (tree exp, rtx target, machine_mode target_mode)
+{
+ if (!validate_arglist (exp, VOID_TYPE))
+return NULL_RTX;
+
+ insn_code icode = direct_optab_handler (fegetround_optab, SImode);
+ if (icode == CODE_FOR_nothing)
+return NULL_RTX;
+
+ if (target == 0
+ || GET_MODE (target) != target_mode
+ || ! (*insn_data[icode].operand[0].predicate) (target,
