Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Hi! On Sun, Oct 04, 2020 at 09:56:01PM -0400, Hans-Peter Nilsson wrote: > Please excuse a comment from the gallery: :-) Thanks! > > > + rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL > > > (operands[1]))); > > This doesn't appear to be very portable, to any-cxx11-compiler > that doesn't pretend to be gcc-intrinsics-compatible. Yup, thanks for spotting this :-) > PS. No less than four targets fail like that. Meh. $ grep -r __builtin_clz config/ config/tilepro/gen-mul-tables.cc: int prev_pow2 = 1 << (31 - __builtin_clz (abs_multiplier)); config/tilepro/gen-mul-tables.cc: 1LL << (63 - __builtin_clzll (abs_multiplier)); This is a generator program that is not run on normal builds, so arguably not a bug. config/nds32/nds32-md-auxiliary.c: clear_sign_bit_copies = __builtin_clz (remainder); config/arc/arc.c: if ((GMASK_LEN - __builtin_clzll (gmask)) == (i + 1) config/arc/arc.c: if ((GMASK_LEN - __builtin_clzll (gmask)) == i Those seem bugs yes. config/rs6000/bmi2intrin.h: c = __builtin_clzl (m); config/rs6000/bmi2intrin.h: c = __builtin_clzl (m); config/rs6000/bmi2intrin.h: c = __builtin_clzl (m); config/rs6000/ppu_intrinsics.h:#define __cntlzw(v) __builtin_clz(v) config/rs6000/ppu_intrinsics.h:#define __cntlzd(v) __builtin_clzll(v) These are installed headers, only used with the built GCC. No bug. config/i386/i386-builtin.def:BDESC (OPTION_MASK_ISA_LZCNT, 0, CODE_FOR_lzcnt_hi, "__builtin_clzs", IX86_BUILTIN_CLZS, UNKNOWN, (int) UINT16_FTYPE_UINT16) A different thing altogether, my grep was a bit wide :-) But not a bug anyway. config/aarch64/aarch64.c: return val == mask * bitmask_imm_mul[__builtin_clz (bits) - 26]; This seems bad as well, yes. Thanks, Segher
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Mon, Oct 26, 2020 at 01:05:00PM -0300, Raoni Fassina Firmino wrote:
> On Mon, Sep 28, 2020 at 11:42:13AM -0500, will schmidt wrote:
> > > +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> > > +;; It doesn't handle values out of range, and always returns 0.
> > > +;; Note that FE_INVALID is unsupported because it maps to more than
> > > +;; one bit on FPSCR register.
> >
> > Should FE_INVALID have an explicit case statement path to FAIL?
>
> Because there is only 4 valid flags I am doing the other way around,
> just checking if it is any of the valid flags and FAIL for any other
> value, so there is no need of an explicit FE_INVALID case, but if it is
> better to have one anyway to make the intention clear through code, I
> don't know.
To clear VX ("invalid", bit 34) you need to clear all different VX bits
(one per cause), so 39-44, 53-55. To *set* it you need to pick which
one you want to set. Maybe this is all best left the the libc in use,
which should have its own policy for that, it might not be the same on
all libcs.
Segher
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Mon, Sep 28, 2020 at 11:42:13AM -0500, will schmidt wrote:
> > +/* Expand call EXP to either feclearexcept or feraiseexcept builtins (from
> > C99
> > +fenv.h), returning the result and setting it in TARGET. Otherwise
> > return
> > +NULL_RTX on failure. */
> > +static rtx
> > +expand_builtin_feclear_feraise_except (tree exp, rtx target,
> > + machine_mode target_mode, optab op_optab)
> > +{
> > + if (!validate_arglist (exp, INTEGER_TYPE, VOID_TYPE))
> > +return NULL_RTX;
> > + rtx op0 = expand_normal (CALL_EXPR_ARG (exp, 0));
> > +
> > + insn_code icode = direct_optab_handler (op_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, target_mode))
> > +target = gen_reg_rtx (target_mode);
> > +
> > + rtx pat = GEN_FCN (icode) (target, op0);
> > + if (!pat)
> > +return NULL_RTX;
> > + emit_insn (pat);
> > +
> > + return target;
> > +}
>
>
> I don't see any references to feclearexcept or feraiseexcept in the
> functions there. I see the callers pass in those values via optab,
> but nothing in these functions explicitly checks or limits that in a
> way that is obvious upon my reading... Thus I wonder if there may be a
> different generic names that would be appropriate for the functions.
Yes, I wonder the same, I guess in theory it could be used with any
int(int) builtin at least, but looking at other more generic
expand_builtin_* I was not confident enough that this one has the
necessary boilerplate code to handle other builtins. Also I looked
aroung builtins.c trying to find an expand that I could reuse and I
notice that the vast majority of builtins use dedicated expand_* so I
just followed suit. I am not really trilled by this name anyway, so If
it something useful in a more generic way (which I don't have enough
knowledge to judge) I am happy to change it.
> > +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> > +;; It doesn't handle values out of range, and always returns 0.
> > +;; Note that FE_INVALID is unsupported because it maps to more than
> > +;; one bit on FPSCR register.
>
> Should FE_INVALID have an explicit case statement path to FAIL?
Because there is only 4 valid flags I am doing the other way around,
just checking if it is any of the valid flags and FAIL for any other
value, so there is no need of an explicit FE_INVALID case, but if it is
better to have one anyway to make the intention clear through code, I
don't know.
> No further comments or nits..
I applied all other suggestions for now,
thanks for the feedback Will :)
o/
Raoni
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Mon, Oct 05, 2020 at 10:36:22AM -0500, Segher Boessenkool wrote: > Should this pattern not allow setting more than one exception bit at > once, btw? Turns out allowing more than one bit was no problem at all. On Mon, Oct 05, 2020 at 10:36:22AM -0500, Segher Boessenkool wrote: > On Sun, Oct 04, 2020 at 09:56:01PM -0400, Hans-Peter Nilsson wrote: > > > > + rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL > > > > (operands[1]))); > > > > This doesn't appear to be very portable, to any-cxx11-compiler > > that doesn't pretend to be gcc-intrinsics-compatible. > > Yeah, very good point! And with that, no ffs() or clz() necessary at all :)
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Hi!
On Sun, Oct 04, 2020 at 09:56:01PM -0400, Hans-Peter Nilsson wrote:
> Please excuse a comment from the gallery:
>
> On Mon, 28 Sep 2020, will schmidt via Gcc-patches wrote:
> > On Fri, 2020-09-04 at 12:52 -0300, Raoni Fassina Firmino via Gcc-patches
> > wrote:
> > > +(define_expand "feraiseexceptsi"
> > > + [(use (match_operand:SI 1 "const_int_operand" "n"))
> > > + (set (match_operand:SI 0 "gpc_reg_operand")
> > > + (const_int 0))]
> > > + "TARGET_HARD_FLOAT"
> > > +{
> > > + switch (INTVAL (operands[1]))
> > > +{
> > > +case 0x200: /* FE_INEXACT */
> > > +case 0x400: /* FE_DIVBYZERO */
> > > +case 0x800: /* FE_UNDERFLOW */
> > > +case 0x1000: /* FE_OVERFLOW */
> > > + break;
> > > +default:
> > > + FAIL;
> > > +}
> > > +
> > > + rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL
> > > (operands[1])));
>
> This doesn't appear to be very portable, to any-cxx11-compiler
> that doesn't pretend to be gcc-intrinsics-compatible.
Yeah, very good point!
Should this pattern not allow setting more than one exception bit at
once, btw?
So you can first see if any out-of-range bits are set:
unsigned int fe = INTVAL (operands[1]);
if ((fe & 0x1e00) != fe)
FAIL;
and then see if more than one bit is set:
if (fe & (fe - 1))
FAIL;
but also disallow zero:
if (!fe)
FAIL;
Or, you can just put the bit number in the switch cases for the four
bits. There are only four, after all.
Thanks,
Segher
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Please excuse a comment from the gallery:
On Mon, 28 Sep 2020, will schmidt via Gcc-patches wrote:
> On Fri, 2020-09-04 at 12:52 -0300, Raoni Fassina Firmino via Gcc-patches
> wrote:
> > 2020-08-13 Raoni Fassina Firmino
> >
> > gcc/ChangeLog:
> > * config/rs6000/rs6000.md (fegetroundsi): New pattern.
> > (feclearexceptsi): New Pattern.
> > (feraiseexceptsi): New Pattern.
> > +(define_expand "feraiseexceptsi"
> > + [(use (match_operand:SI 1 "const_int_operand" "n"))
> > + (set (match_operand:SI 0 "gpc_reg_operand")
> > + (const_int 0))]
> > + "TARGET_HARD_FLOAT"
> > +{
> > + switch (INTVAL (operands[1]))
> > +{
> > +case 0x200: /* FE_INEXACT */
> > +case 0x400: /* FE_DIVBYZERO */
> > +case 0x800: /* FE_UNDERFLOW */
> > +case 0x1000: /* FE_OVERFLOW */
> > + break;
> > +default:
> > + FAIL;
> > +}
> > +
> > + rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL
> > (operands[1])));
This doesn't appear to be very portable, to any-cxx11-compiler
that doesn't pretend to be gcc-intrinsics-compatible.
If the valid values had been more complicated, there'd be
bitmap.c:bitmap_last_set_bit to follow for a suitable portable
pattern. It conditionalizes like:
#if GCC_VERSION >= 3004
gcc_assert (sizeof (long) == sizeof (word));
bit_no += BITMAP_WORD_BITS - __builtin_clzl (word) - 1;
#else
...
#endif
Better, there's "ffs".
brgds, H-P
PS. No less than four targets fail like that. Meh.
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Fri, 2020-09-04 at 12:52 -0300, Raoni Fassina Firmino via Gcc-patches wrote: > Changes since v1[1]: > - Fixed english spelling; > - Fixed code-style; > - Changed match operand predicate in feclearexcept and feraiseexcept; > - Changed testcase options; > - Minor changes in test code to be C90 compatible; > - Other minor changes sugested by Segher; > - Changed subject line tag (not sure if I tagged correctly or should > include optabs: also) > > There is one pending question raised by Segher, It is about adding > documentation, I am not sure if it is needed and if so, where it > should be. I will quote the relevant part of the conversation[2] from > the v1 thread for context: > > > > > +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. > > tested on top of master (c5a6c2237a1156dc43fa32c938fc32acdfc2bff9) > 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-August/552024.html > [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553295.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. > A few cosmetic nits below,.. this is perhaps enough activity to get others to take a look. :-) > Signed-off-by: Raoni Fassina Firmino > > FIX: patch_v1 (2) > --- > gcc/builtins.c| 76 ++ > gcc/config/rs6000/rs6000.md | 82 +++ > gcc/optabs.def| 4 + > .../builtin-feclearexcept-feraiseexcept-1.c | 68 + > .../builtin-feclearexcept-feraiseexcept-2.c | 133 ++ > .../gcc.target/powerpc/builtin-fegetround.c | 36 + > 6 files changed, 399 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 97f1a184dc6..a6f6141edb7 100644 > --- a/gcc/builtins.c > +++ b/gcc/builtins.c > @@ -115,6 +115,9 @@ 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_r
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Ping.
Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Ping.
