[v3] powerpc: Refactor fenvinline.h

  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(-)
  

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?
  

* 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
  

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?
>
  

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!