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
