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 0x020000000 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 <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).
The rs6000 parts (including the testcases) look fine other than those
things. Please fix and resend?
For the generic parts someone else will have to review it (it looks fine
to me, if that helps).
Segher
