Message ID | 20200210150902.129948-1-rcardoso@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v3] powerpc: Refactor fenvinline.h | expand |
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!
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); \