diff mbox

[v3] powerpc: Refactor fenvinline.h

Message ID 20200210150902.129948-1-rcardoso@linux.ibm.com
State Superseded
Delegated to: Tulio Magno Quites Machado Filho
Headers show

Commit Message

Rogerio Alves Feb. 10, 2020, 3:09 p.m. UTC
This patch refactor fenviline.h replaces some statements for
builtins.
---
    * Changes in v3: Per Paul Clark review: replace __mtfsb[01] for
    __MTFSB[01] to avoid name clashing with XL compiler (XL already
    defined __mtfsb.
   
    * Changes in v2: Per Adhemerval review: don not redefine _buitins use a
    macro instead. Use of __builtin_popcount allows do eliminate the
    if (e == 0) test. __builtin_popcount(e) == 1 for e = 0 validate false 
    while (e & (e - 1)) == 0 validate true if e = 0. Appended a comment
    about the weird asm constrain i#*X. The warning only appear at GCC
    versions 3 and below. If we even stops to support GCC 3 for installed
    headers we may replace that for plain `i`.

 sysdeps/powerpc/bits/fenvinline.h | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

Comments

Tulio Magno Quites Machado Filho Feb. 25, 2020, 6:23 p.m. UTC | #1
Rogerio Alves <rcardoso@linux.ibm.com> writes:

>  /* The weird 'i#*X' constraints on the following suppress a gcc
>     warning when __excepts is not a constant.  Otherwise, they mean the
> -   same as just plain 'i'.  */
> +   same as just plain 'i'. This warning only happens in old GCC 

2 spaces here ---------------^     trailing whitespace here ------^

> +   versions (gcc 3 or less). Otherwise plain 'i' works fine.  */

Likewise ----------------------^

> -	if (__e != 0)							      \
> -	  __asm__ __volatile__ ("mtfsb1 %0"				      \
> -				: : "i#*X" (__builtin_clz (__e)));	      \
> -        __ret = 0;							      \
> +	  __MTFSB1 ((__builtin_clz (__e)));				      \

Wrong indentation.

>          && __e != FE_INVALID)						      \
>        {									      \
> -	if (__e != 0)							      \
> -	  __asm__ __volatile__ ("mtfsb0 %0"				      \
> -				: : "i#*X" (__builtin_clz (__e)));	      \
> -        __ret = 0;							      \
> +	  __MTFSB0 ((__builtin_clz (__e)));				      \

Wrong indentation.

> @@ -80,15 +87,12 @@
>  #   define feclearexcept(__excepts) \
>    (__extension__  ({ 							      \
>      int __e = __excepts;						      \
> -    int __ret;								      \
> +    int __ret = 0;							      \
>      if (__builtin_constant_p (__e)					      \
> -        && (__e & (__e - 1)) == 0					      \
> +        && (__builtin_popcount (__e) == 1)				      \

I remember that Adhemerval disagreed with this particular change and I
don't remember seeing any agreement there.
Rogerio, if you agree with the removal of these lines I can remove this change
from the patch, fix the cosmetic issues and push the patch.
Does it look good for you?
Florian Weimer Feb. 25, 2020, 6:48 p.m. UTC | #2
* Tulio Magno Quites Machado Filho:

>> +    int __ret = 0;							      \
>>      if (__builtin_constant_p (__e)					      \
>> -        && (__e & (__e - 1)) == 0					      \
>> +        && (__builtin_popcount (__e) == 1)				      \
>
> I remember that Adhemerval disagreed with this particular change and I
> don't remember seeing any agreement there.

This is the powerof2 macro, by the way.

Thanks,
Florian
Adhemerval Zanella Feb. 25, 2020, 8:03 p.m. UTC | #3
On 25/02/2020 15:23, Tulio Magno Quites Machado Filho wrote:
> Rogerio Alves <rcardoso@linux.ibm.com> writes:
> 
>>  /* The weird 'i#*X' constraints on the following suppress a gcc
>>     warning when __excepts is not a constant.  Otherwise, they mean the
>> -   same as just plain 'i'.  */
>> +   same as just plain 'i'. This warning only happens in old GCC 
> 
> 2 spaces here ---------------^     trailing whitespace here ------^
> 
>> +   versions (gcc 3 or less). Otherwise plain 'i' works fine.  */
> 
> Likewise ----------------------^
> 
>> -	if (__e != 0)							      \
>> -	  __asm__ __volatile__ ("mtfsb1 %0"				      \
>> -				: : "i#*X" (__builtin_clz (__e)));	      \
>> -        __ret = 0;							      \
>> +	  __MTFSB1 ((__builtin_clz (__e)));				      \
> 
> Wrong indentation.
> 
>>          && __e != FE_INVALID)						      \
>>        {									      \
>> -	if (__e != 0)							      \
>> -	  __asm__ __volatile__ ("mtfsb0 %0"				      \
>> -				: : "i#*X" (__builtin_clz (__e)));	      \
>> -        __ret = 0;							      \
>> +	  __MTFSB0 ((__builtin_clz (__e)));				      \
> 
> Wrong indentation.
> 
>> @@ -80,15 +87,12 @@
>>  #   define feclearexcept(__excepts) \
>>    (__extension__  ({ 							      \
>>      int __e = __excepts;						      \
>> -    int __ret;								      \
>> +    int __ret = 0;							      \
>>      if (__builtin_constant_p (__e)					      \
>> -        && (__e & (__e - 1)) == 0					      \
>> +        && (__builtin_popcount (__e) == 1)				      \
> 
> I remember that Adhemerval disagreed with this particular change and I
> don't remember seeing any agreement there.

My reservation is the change is just an idiomatic one, it is not either
simplifying the code or optimizing it. I am ok with the usage of 
powerof2, as Florian suggested.

> Rogerio, if you agree with the removal of these lines I can remove this change
> from the patch, fix the cosmetic issues and push the patch.
> Does it look good for you?
>
Tulio Magno Quites Machado Filho Feb. 25, 2020, 11:15 p.m. UTC | #4
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> My reservation is the change is just an idiomatic one, it is not either
> simplifying the code or optimizing it. I am ok with the usage of 
> powerof2, as Florian suggested.

Great!  I made this change, fixed the other issues and pushed it as
f1a0840c15d039631c13258544cdc04e4cbb9c69.

Thanks!
diff mbox

Patch

diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
index 70689664e2..fec69b5a02 100644
--- a/sysdeps/powerpc/bits/fenvinline.h
+++ b/sysdeps/powerpc/bits/fenvinline.h
@@ -51,9 +51,19 @@ 
 # define fegetround() __fegetround ()
 
 # ifndef __NO_MATH_INLINES
+
+/* Builtins to mtfsb0 and mtfsb1 was introduced on GCC 9.  */
+#  if !__GNUC_PREREQ(9, 0)
 /* The weird 'i#*X' constraints on the following suppress a gcc
    warning when __excepts is not a constant.  Otherwise, they mean the
-   same as just plain 'i'.  */
+   same as just plain 'i'. This warning only happens in old GCC 
+   versions (gcc 3 or less). Otherwise plain 'i' works fine.  */
+#   define __MTFSB0(__b) __asm__ __volatile__ ("mtfsb0 %0" : : "i#*X" (__b))
+#   define __MTFSB1(__b) __asm__ __volatile__ ("mtfsb1 %0" : : "i#*X" (__b))
+#  else
+#   define __MTFSB0(__b) __builtin_mtfsb0 (__b)
+#   define __MTFSB1(__b) __builtin_mtfsb1 (__b)
+#  endif
 
 #  if __GNUC_PREREQ(3, 4)
 
@@ -61,15 +71,12 @@ 
 #   define feraiseexcept(__excepts) \
   (__extension__  ({ 							      \
     int __e = __excepts;						      \
-    int __ret;								      \
+    int __ret = 0;							      \
     if (__builtin_constant_p (__e)					      \
-        && (__e & (__e - 1)) == 0					      \
+        && (__builtin_popcount (__e) == 1)				      \
         && __e != FE_INVALID)						      \
       {									      \
-	if (__e != 0)							      \
-	  __asm__ __volatile__ ("mtfsb1 %0"				      \
-				: : "i#*X" (__builtin_clz (__e)));	      \
-        __ret = 0;							      \
+	  __MTFSB1 ((__builtin_clz (__e)));				      \
       }									      \
     else								      \
       __ret = feraiseexcept (__e);					      \
@@ -80,15 +87,12 @@ 
 #   define feclearexcept(__excepts) \
   (__extension__  ({ 							      \
     int __e = __excepts;						      \
-    int __ret;								      \
+    int __ret = 0;							      \
     if (__builtin_constant_p (__e)					      \
-        && (__e & (__e - 1)) == 0					      \
+        && (__builtin_popcount (__e) == 1)				      \
         && __e != FE_INVALID)						      \
       {									      \
-	if (__e != 0)							      \
-	  __asm__ __volatile__ ("mtfsb0 %0"				      \
-				: : "i#*X" (__builtin_clz (__e)));	      \
-        __ret = 0;							      \
+	  __MTFSB0 ((__builtin_clz (__e)));				      \
       }									      \
     else								      \
       __ret = feclearexcept (__e);					      \