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 0x020000000 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, __builtin_clz
> > (INTVAL(operands[1])));
>
> Space after "INTVAL".
Done.
>
> > + 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?
Done.
>
> > +(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).
Done.
>
> > +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) and I didn't find a place where i feel it
sits well. On the PowerPC target specific sections (6.60.22 Basic
PowerPC Built-in Functions), I didn't found it mentioning builtins that
are optimizations for the standard library functions, but we do have
many of these for Power. Then, on the generic section (6.59 Other
Built-in Functions Provided by GCC) it mentions C99 functions that have
builtins but it seems like it mentions builtins that have target
independent implementation, or at least it dos not say that some
builtins may be implemented on only some targets. And in this case
there is no implementation (for now) for any other target that is not
PowerPc.
So, I don't know if or where this should be documented.
>
> > --- /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.
>
> > +/* { 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?
>
> > +#include <fenv.h>
>
> That header does not exist everywhere. You can just declare the things
> you need (the FE_ constants?)
>
> Or perhaps you want to include
>
> /* { dg-require-effective-target fenv_exceptions } */
>
> (probably easiest and nicest, I think).
Done.
>
> The rs6000 parts (including the testcases) look fine other than those
> things. Please fix and resend?
Will do.
>
> For the generic parts someone else will have to review it (it looks fine
> to me, if that helps).
>
>
> Segher
Thanks,
o/
Raoni
