Re: [PATCH v5] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]

[Segher Boessenkool <segher@kernel.crashing.org>]

On Wed, Nov 18, 2020 at 08:31:28AM +0100, Richard Biener wrote:
> On Tue, 17 Nov 2020, Jeff Law wrote:
> > On 11/4/20 8:10 AM, Raoni Fassina Firmino via Gcc-patches wrote:
> > > On Wed, Nov 04, 2020 at 10:35:03AM +0100, Richard Biener wrote:
> > >>> +/* Expand call EXP to the fegetround builtin (from C99 fenv.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, target_mode))
> > >>> +    target = gen_reg_rtx (target_mode);
> > >>> +
> > >>> +  rtx pat = GEN_FCN (icode) (target);
> > >>> +  if (!pat)
> > >>> +    return NULL_RTX;
> > >>> +  emit_insn (pat);
> > >> I think you need to verify whether the expansion ended up in 'target'
> > >> and otherwise emit a move since usually 'target' is just a hint.
> > > I thought the "if (target == 0 ..." took care of that. The expands do
> > > emit a move, if that helps.
> > It looks like if we have a passed in target and it either has the wrong
> > mode or it does not match the predicate, then we generaet a new target
> > and use that instead.? I don't see where we'd copy from that new target
> > to the original desired target.? For some expanders the caller would
> > handle that, but I don't see how that's possible for this one without
> > the caller digging into the generated RTL to determine that
> > expand_builtin_fegetround put the result somewhere other than TARGET and
> > thus a copy is needed.
> > 
> > That may be what Richi is worried about.
> 
> I know we've added missing
> 
>   if (!rtx_equal_p (target, ops[0].value))
>     emit_move_insn (target, ops[0].value);
> 
> to several expanders (using expand_insn rather than manual
> GEN_FCN (icode) calls).

We can handle the constants issue similarly to what we do for
__builtin_fpclassify, too.


Segher