[v3] powerpc: Refactor fenvinline.h
Commit Message
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
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!
@@ -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); \