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

[Jeff Law <law@redhat.com>]

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.
>
> For feclearexcept and feraiseexcept I included tests to variable
> 'target', including none, but now I see that I did not do the same for
> fegetround, I can add the same if it is necessary, but the test do check
> if the return is correct, so I don't know.
>
>
>>> +@cindex @code{fegetround@var{m}} instruction pattern
>>> +@item @samp{fegetround@var{m}}
>>> +Store the current machine floating-point rounding mode into operand 0.
>>> +Operand 0 has mode @var{m}, which is scalar.  This pattern is used to
>>> +implement the @code{fegetround} function from the ISO C99 standard.
>> I think this needs to elaborate on the format of the "rounding mode".
>>
>> AFAICS you do nothing to marshall with the actually used libc
>> implementation which AFAIU can choose arbitrary values for
>> the FE_* macros.  I'm not sure we require the compiler to be
>> configured for one specific C library and for example require
>> matching FE_* macro definitions for all uses of the built
>> compiler.
>>
>> For the patch at hand you seem to assume the libc "format"
>> matches the hardware one (which would of course be reasonable).
>>
>> Does that actually hold up when looking at libcs other than 
>> glibc supporting powerpc?
> I checked in some other libc implementations that have POWER support and
> all have the same value as glic for the four rounding modes and the five
> exception flags from libc. The libcs implementations I checked are:
>
>  - musl
>  - uclibc & uclibc-ng
>  - freebsd
>
> Is There any other I am missing?
I think the concern here is that while the libcs we have visibility into
have consistent values, I don't think that's necessarily guaranteed.  
I'm not sure how to DTRT here.  We may not have the target headers if
we're doing a cross compile, so a configure test may not do what we 
need.  In fact, ISTM that there is no reliable configure or compile time
check we can do since the constants are part of the runtime and can
change independently of the compiler.

Jeff