Re: [PATCH v2] rs6000: Add buildin for mffscrn instructions

["Kewen.Lin" <linkw@linux.ibm.com>]

on 2023/5/23 01:31, Carl Love wrote:
> On Mon, 2023-05-22 at 14:36 +0800, Kewen.Lin wrote:
>> Hi Carl,
>>
>> on 2023/5/19 05:12, Carl Love via Gcc-patches wrote:
>>> GCC maintainers:
>>>
>>> version 2.  Fixed an issue with the test case.  The dg-options line
>>> was
>>> missing.
>>>
>>> The following patch adds an overloaded builtin.  There are two
>>> possible
>>> arguments for the builtin.  The builtin definitions are:
>>>
>>>   double __builtin_mffscrn (unsigned long int);
>>>   double __builtin_mffscrn (double);
>>>
>>
>> We already have one  bif __builtin_set_fpscr_rn for RN setting,
>> apparently
>> these two are mainly for direct mapping to mffscr[ni] and want the
>> FPSCR bits.
>> I'm curious what's the requirements requesting these two built-in
>> functions?
> 
> The builtins were requested for use in GLibC.  As of version 2.31 they
> were added as inline asm.  They requested a builtin so the asm could be
> removed.
> 

OK, thanks for the information.

>>
>>> The patch has been tested on Power 10 with no regressions.  
>>>
>>> Please let me know if the patch is acceptable for
>>> mainline.  Thanks.
>>>
>>>                     Carl
>>>
>>> ------------------------------------------------
>>> rs6000: Add buildin for mffscrn instructions
>>>
>>
>> s/buildin/built-in/
> 
> fixed
>>
>>> This patch adds overloaded __builtin_mffscrn for the move From
>>> FPSCR
>>> Control & Set R instruction with an immediate argument.  It also
>>> adds the
>>> builtin with a floating point register argument.  A new runnable
>>> test is
>>> added for the new builtin.
>>
>> s/Set R/Set RN/
> 
> fixed
> 
>>> gcc/
>>>
>>> 	* config/rs6000/rs6000-builtins.def (__builtin_mffscrni,
>>> 	__builtin_mffscrnd): Add builtin definitions.
>>> 	* config/rs6000/rs6000-overload.def (__builtin_mffscrn): Add
>>> 	overloaded definition.
>>> 	* doc/extend.texi: Add documentation for __builtin_mffscrn.
>>>
>>> gcc/testsuite/
>>>
>>> 	* gcc.target/powerpc/builtin-mffscrn.c: Add testcase for new
>>> 	builtin.
>>> ---
>>>  gcc/config/rs6000/rs6000-builtins.def         |   7 ++
>>>  gcc/config/rs6000/rs6000-overload.def         |   5 +
>>>  gcc/doc/extend.texi                           |   8 ++
>>>  .../gcc.target/powerpc/builtin-mffscrn.c      | 106
>>> ++++++++++++++++++
>>>  4 files changed, 126 insertions(+)
>>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-
>>> mffscrn.c
>>>
>>> diff --git a/gcc/config/rs6000/rs6000-builtins.def
>>> b/gcc/config/rs6000/rs6000-builtins.def
>>> index 92d9b46e1b9..67125473684 100644
>>> --- a/gcc/config/rs6000/rs6000-builtins.def
>>> +++ b/gcc/config/rs6000/rs6000-builtins.def
>>> @@ -2875,6 +2875,13 @@
>>>    pure vsc __builtin_vsx_xl_len_r (void *, signed long);
>>>      XL_LEN_R xl_len_r {}
>>>  
>>> +; Immediate instruction only uses the least significant two bits
>>> of the
>>> +; const int.
>>> +  double __builtin_mffscrni (const int<2>);
>>> +    MFFSCRNI rs6000_mffscrni {}
>>> +
>>> +  double __builtin_mffscrnd (double);
>>> +    MFFSCRNF rs6000_mffscrn {}
>>>  
>>
>> Why are them put in [power9-64] rather than [power9]?  IMHO [power9]
>> is the
>> correct stanza for them.
> 
> Moved them to power 9 stanza.
> 
>>   Besides, {nosoft} attribute is required.
> 
> OK, added that.  I was trying to figure out why nosoft is needed.  The
> instructions are manipulating bits in a physical register that controls
> the hardware floating point instructions.  It looks to me like that
> would be why.  Because if you were using msoft float then the floating
> point HW registers are disabled and the floating point operations are
> done using software.  Did I figure this out correctly?

Yes, and also the destination of these two instructions is hardware float
register, its relatives mffs and mffsl have that as well.

> 
>  
>>
>>>  ; Builtins requiring hardware support for IEEE-128 floating-point.
>>>  [ieee128-hw]
>>> diff --git a/gcc/config/rs6000/rs6000-overload.def
>>> b/gcc/config/rs6000/rs6000-overload.def
>>> index c582490c084..adda2df69ea 100644
>>> --- a/gcc/config/rs6000/rs6000-overload.def
>>> +++ b/gcc/config/rs6000/rs6000-overload.def
>>> @@ -78,6 +78,11 @@
>>>  ; like after a required newline, but nowhere else.  Lines
>>> beginning with
>>>  ; a semicolon are also treated as blank lines.
>>>  
>>> +[MFFSCR, __builtin_mffscrn, __builtin_mffscrn]
>>> +  double __builtin_mffscrn (const int<2>);
>>> +    MFFSCRNI
>>> +  double __builtin_mffscrn (double);
>>> +    MFFSCRNF
>>>  
>>>  [BCDADD, __builtin_bcdadd, __builtin_vec_bcdadd]
>>>    vsq __builtin_vec_bcdadd (vsq, vsq, const int);
>>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>>> index ed8b9c8a87b..f16c046051a 100644
>>> --- a/gcc/doc/extend.texi
>>> +++ b/gcc/doc/extend.texi
>>> @@ -18455,6 +18455,9 @@ int __builtin_dfp_dtstsfi_ov_td (unsigned
>>> int comparison, _Decimal128 value);
>>>  
>>>  double __builtin_mffsl(void);
>>>  
>>> +double __builtin_mffscrn (unsigned long int);
>>> +double __builtin_mffscrn (double);
>>
>> s/unsigned long int/const int/
> 
> Fixed
> 
>>
>> Note that this section is for all configurations and your
>> implementation is put
>> __builtin_mffscrn power9 only, so if the intention (requirement) is
>> to make this
>> be for also all configurations, we need to deal with the cases
>> without the support
>> of actual hw insns mffscrn{,i}, just like the existing handlings for
>> mffsl etc.
> 
> I think it should be sufficient to just support these when floating
> hardware instructions are supported.  So I believe I just need to move
> these to the correct place in the document.  I think that means they
> should be in the section:
> 
>   The following functions require @option{-mhard-float},
>   @option{-mpowerpc-gfxopt}, and @option{-mpopcntb} options.
> 
> Moved to the end of the above section.  Hope that is correct.

The hardware insn mffsl which is available starting from ISA 3.0, but
the related built-in __builtin_mffsl is available on all configurations,
since it has the falling back as rs6000-builtins.def said:

; Although the mffsl instruction is only available on POWER9 and later
; processors, this builtin automatically falls back to mffs on older
; platforms.  Thus it appears here in the [always] stanza.
  double __builtin_mffsl ();
    MFFSL rs6000_mffsl {nosoft}

So IMHO we also want the similar support for mffscrn, that is to make
use of mffscrn and mffscrni on Power9 and later, but falls back to 
__builtin_set_fpscr_rn + mffs similar on older platforms.

Segher & Peter, what do you think of this?

BR,
Kewen