Re: [PATCH] [powerpc] fegetenv_and_set_rn now uses the builtins provided by GCC.

[Manjunath S Matti <mmatti@linux.vnet.ibm.com>]

On 12/09/23 6:57 pm, Adhemerval Zanella Netto wrote:
>
> On 12/09/23 05:27, Manjunath Matti wrote:
>> On powerpc, SET_RESTORE_ROUND uses inline assembly to optimize the
>> prologue get/save/set rounding mode operations for POWER9 and
>> later by using 'mffscrn' where possible, this was introduced by
>> commit f1c56cdff09f650ad721fae026eb6a3651631f3d.
>>
>> GCC version 14 onwards supports builtins as __builtin_set_fpscr_rn
>> which now returns the FPSCR fields in a double. This feature is
>> available on Power9 when the __SET_FPSCR_RN_RETURNS_FPSCR__ macro
>> is defined along with __builtin_set_fpscr_rn enabled.
>> GCC commit ef3bbc69d15707e4db6e2f198c621effb636cc26 adds
>> this feature.
>>
>> Changes are done to use __builtin_set_fpscr_rn instead of mffscrn
>> or mffscrni in __fe_mffscrn(rn).
>>
>> Suggested-by: Carl Love <cel@us.ibm.com>
>> ---
>>   sysdeps/powerpc/fpu/fenv_libc.h | 23 ++++++++++++++++++++---
>>   1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h
>> index fa5e1c697e..55484eb229 100644
>> --- a/sysdeps/powerpc/fpu/fenv_libc.h
>> +++ b/sysdeps/powerpc/fpu/fenv_libc.h
>> @@ -84,8 +84,15 @@ extern const fenv_t *__fe_mask_env (void) attribute_hidden;
>>       __fr.fenv;								\
>>     })
>>   
>> +/* GCC version 14 onwards supports builtins as __builtin_set_fpscr_rn and
>> +   now returns the FPSCR fields in a double. This support is available
>> +   on Power9 when the __SET_FPSCR_RN_RETURNS_FPSCR__ macro is defined.
>> +   To retain backward compatibility with older GCC, we still retain the
>> +   old inline assembly implementation.  */
>> +#if defined _ARCH_PWR9 && defined __SET_FPSCR_RN_RETURNS_FPSCR__
>> +#define fegetenv_and_set_rn(rn) __builtin_set_fpscr_rn (rn)
>> +#elif defined _ARCH_PWR9
>>   /* Like fegetenv_control, but also sets the rounding mode.  */
>> -#ifdef _ARCH_PWR9
>>   #define fegetenv_and_set_rn(rn) __fe_mffscrn (rn)
>>   #else
>>   /* 'mffscrn' will decode to 'mffs' on ARCH < 3_00, which is still necessary
> I think the macro would be better defined as:
>
> #ifdef __SET_FPSCR_RN_RETURNS_FPSCR__
> # define __fe_mffscrn(rn)  __builtin_set_fpscr_rn (rn)
> #else
> # define __fe_mffscrn(rn) [...]
> #endif
>
> Then there is no need to redefine fegetenv_and_set_rn nor __fesetround_inline.

So this is what you are asking me to do right ?

--- a/sysdeps/powerpc/fpu/fenv_libc.h
+++ b/sysdeps/powerpc/fpu/fenv_libc.h
@@ -89,22 +89,11 @@ extern const fenv_t *__fe_mask_env (void) 
attribute_hidden;
     on Power9 when the __SET_FPSCR_RN_RETURNS_FPSCR__ macro is defined.
     To retain backward compatibility with older GCC, we still retain the
     old inline assembly implementation.  */
-#if defined _ARCH_PWR9 && defined __SET_FPSCR_RN_RETURNS_FPSCR__
+#ifdef __SET_FPSCR_RN_RETURNS_FPSCR__
  #define fegetenv_and_set_rn(rn) __builtin_set_fpscr_rn (rn)
-#elif defined _ARCH_PWR9
+#else
  /* Like fegetenv_control, but also sets the rounding mode.  */
  #define fegetenv_and_set_rn(rn) __fe_mffscrn (rn)
-#else
-/* 'mffscrn' will decode to 'mffs' on ARCH < 3_00, which is still necessary
-   but not sufficient, because it does not set the rounding mode.
-   Explicitly set the rounding mode when 'mffscrn' actually doesn't.  */
-#define fegetenv_and_set_rn(rn) \
-  ({register fenv_union_t 
__fr;                                                \
-    __fr.fenv = __fe_mffscrn (rn);                                     \
-    if (__glibc_unlikely (!(GLRO(dl_hwcap2) & 
PPC_FEATURE2_ARCH_3_00)))        \
-      __fesetround_inline 
(rn);                                                \
- __fr.fenv; \
-  })
  #endif


I was under the impression that the redefine of fegetenv_and_set_rn and 
__fesetround_inline

was needed for architectures below POWE9, i.e POWER8, 7, etc.

Please correct me if I am wrong.

>> @@ -148,7 +155,12 @@ typedef union
>>   static inline int
>>   __fesetround_inline (int round)
>>   {
>> -#ifdef _ARCH_PWR9
>> +/* GCC version 14 onwards supports builtins as __builtin_set_fpscr_rn and
>> +   now returns the FPSCR fields in a double. This support is available
>> +   on Power9 when the __SET_FPSCR_RN_RETURNS_FPSCR__ macro is defined.  */
>> +#if defined _ARCH_PWR9 && defined __SET_FPSCR_RN_RETURNS_FPSCR__
>> +  __builtin_set_fpscr_rn (round);
>> +#elif defined _ARCH_PWR9
>>     __fe_mffscrn (round);
>>   #else
>>     if (__glibc_likely (GLRO(dl_hwcap2) & PPC_FEATURE2_ARCH_3_00))
>> @@ -178,7 +190,12 @@ __fesetround_inline (int round)
>>   static inline void
>>   __fesetround_inline_nocheck (const int round)
>>   {
>> -#ifdef _ARCH_PWR9
>> +/* GCC version 14 onwards supports builtins as __builtin_set_fpscr_rn and
>> +   now returns the FPSCR fields in a double. This support is available
>> +   on Power9 when the __SET_FPSCR_RN_RETURNS_FPSCR__ macro is defined.  */
>> +#if defined _ARCH_PWR9 && defined __SET_FPSCR_RN_RETURNS_FPSCR__
>> +  __builtin_set_fpscr_rn (round);
>> +#elif defined _ARCH_PWR9
>>     __fe_mffscrn (round);
>>   #else
>>     if (__glibc_likely (GLRO(dl_hwcap2) & PPC_FEATURE2_ARCH_3_00))


So the rest it OK ? I will update the patch with suggested changes.