Re: [PATCH] powerpc: Fix feraiseexcept and feclearexcept macros

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 03/03/2020 15:20, Matheus Castanho wrote:
> A recent change to fenvinline.h modified the check if __e is a
> a power of 2 inside feraiseexcept and feclearexcept macros.  It
> introduced the use of the powerof2 macro but also removed the
> if statement checking whether __e != 0 before issuing an mtfsb*
> instruction.  This is problematic because powerof2 (0) evaluates
> to 1 and without the removed if __e is allowed to be 0 when
> __builtin_clz is called.  In that case the value 32 is passed
> to __MTFSB*, which is invalid.
> 
> This commit uses __builtin_popcount instead of powerof2 to fix this
> issue and avoid the extra check for __e != 0.  This was the approach
> used by the initial versions of that previous patch.

This code is becoming convoluted and I think these micro-optimization
are hardly wildly used and even more being a possible hotspot in 
realword cases (non-default rounding are used only on specific cases 
and excepting handling are done most likely only on exceptions cases).

So, I think we should just remove such optimization as we have done for
string{2,3}.h and leverage the compiler with some builtin if it were 
the case. It would allow also remove one extra unused installed header
file, since it seems that only powerpc push for this kind of 
optimization.

For the patch itself, I don't have a strong opinion. Either
adding the '__e == 0' check back or using your suggestion would
be fine.

> ---
>  sysdeps/powerpc/bits/fenvinline.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
> index 89ea38a4f8..f2d095a72f 100644
> --- a/sysdeps/powerpc/bits/fenvinline.h
> +++ b/sysdeps/powerpc/bits/fenvinline.h
> @@ -75,7 +75,7 @@
>      int __e = __excepts;						      \
>      int __ret = 0;							      \
>      if (__builtin_constant_p (__e)					      \
> -        && powerof2 (__e)						      \
> +        && __builtin_popcount (__e) == 1				      \
>          && __e != FE_INVALID)						      \
>        {									      \
>  	__MTFSB1 ((__builtin_clz (__e)));				      \
> @@ -91,7 +91,7 @@
>      int __e = __excepts;						      \
>      int __ret = 0;							      \
>      if (__builtin_constant_p (__e)					      \
> -        && powerof2 (__e)						      \
> +        && __builtin_popcount (__e) == 1				      \
>          && __e != FE_INVALID)						      \
>        {									      \
>  	__MTFSB0 ((__builtin_clz (__e)));				      \
>