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
