[v2] powerpc: Refactor fenvinline.h

  This patch refactor fenviline.h replaces some statements for
builtins.

---

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

On 2/7/20 8:34 AM, Rogerio Alves wrote:
> This patch refactor fenviline.h replaces some statements for
> builtins.
> 
> ---
> 
>  * 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(-)
> 
> diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
> index 70689664e2..7b2b75e5ef 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

These names actually conflict with XL compiler builtins which serve the same purpose.  https://www.ibm.com/support/knowledgecenter/SSXVZZ_16.1.1/com.ibm.xlcpp1611.lelinux.doc/compiler_ref/bifs_fpscr.html

Maybe all caps?  __MTFSB0/__MTFSB1 ?

Of course, I don't think XL currently provides the __builtin versions, so the chances of an actual collision are vanishingly small.  I would be nice to plan for better compatibility in the future, though.  :-)

PC
  

Em 07/02/2020 11:50, Paul Clarke escreveu:
> On 2/7/20 8:34 AM, Rogerio Alves wrote:
>> This patch refactor fenviline.h replaces some statements for
>> builtins.
>>
>> ---
>>
>>   * 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(-)
>>
>> diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
>> index 70689664e2..7b2b75e5ef 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
> 
> These names actually conflict with XL compiler builtins which serve the same purpose.  https://www.ibm.com/support/knowledgecenter/SSXVZZ_16.1.1/com.ibm.xlcpp1611.lelinux.doc/compiler_ref/bifs_fpscr.html
> 
> Maybe all caps?  __MTFSB0/__MTFSB1 ?
> 
Weird. I've tested with XL and it did not give any warning. But I was 
not aware about that. Thank you for point me that out. I will change it 
to a caps version. I guess it will be fine.

> Of course, I don't think XL currently provides the __builtin versions, so the chances of an actual collision are vanishingly small.  I would be nice to plan for better compatibility in the future, though.  :-)
> 
> PC
>