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

[Raoni Fassina Firmino <raoni@linux.ibm.com>]

It seems to me we have two unrelated concerns mixed in the threads, I
will reply in two different sub-threads to make this easier.

This one to discuss the handling of target and output from the expands


On Wed, Nov 18, 2020 at 02:45:44PM -0700, AL gcc-patches wrote:
> 
> 
> On 11/18/20 12:31 AM, 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).
> Yes.  But I think we end up doing that mostly for expanders that return
> the object where the value was stored in some reasonably convenient
> location (either as a return value or in an ops array).  I don't think
> that's the case here. 

So, I think I got it wrong then, I thought the semantics where that
the expander was responsible to provide a suitable target to the
expand, and the expand was responsible to output to that target.  That
is how I created both, so if the expand can change the target maybe
then it should be also responsible to generate the correct target.
But this seems to me that we will have more repeated code for expands
in different archs.


o/
Raoni Fassina