Re: [PATCH] powerpc: fenvinline.h refactor

[Rogerio Alves <rcardoso at linux dot ibm dot com>]

> Subject: Re: [PATCH] powerpc: fenvinline.h refactor > Date: Thu, 23 Jan 2020 18:24:38 -0300
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> To: libc-alpha@sourceware.org
>
>
>
> On 23/01/2020 15:14, Rogerio Alves wrote:
> > Hi,
> >
> > Attached to this email is a patch to refactor fenvinline.h for power to include some __buitins when available. I also removed a weird asm constraint. I can't reproduce the warning described on the file and the test passed. Not sure why the put that constraint. I will appreciate any reviews.
>
> PS: please send the patch inline next time, it helps the review
> process.

Ok. Once I include you suggestions I will send a inline patch instead.

> > Regard
>
> >  From dbeca754bcbc6c77b6157a2f818554db31bac2ba Mon Sep 17 00:00:00 2001
> > From: Rogerio Alves <rcardoso@linux.ibm.com>
> > Date: Thu, 23 Jan 2020 14:54:00 -0300
> > Subject: [PATCH v1] powerpc: Refactor fenvinline.h
> >
> > This patch refactor fenviline.h replaces some statements for
> > builtins.
> >
> > 2020-01-22  Rogerio A. Cardoso  <rcardoso@linux.ibm.com>
> >
> > 	* sysdeps/unix/sysv/linux/powerpc/bits/fenvline.h:
> >      Replace expression by __builtin_popcount. Use
> >      __builtin_mtfsb[01] when avaliable. Remove weird i#*X
> >      asm constraint.
>
> No ChangeLog entry required anymore.

Oh. I was not aware. Last time I sent a patch here was still required. I 
will remove.

> > ---
> >   sysdeps/powerpc/bits/fenvinline.h | 25 +++++++++++++++----------
> >   1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
> > index 70689664e2..ae04ece189 100644
> > --- a/sysdeps/powerpc/bits/fenvinline.h
> > +++ b/sysdeps/powerpc/bits/fenvinline.h
> > @@ -15,7 +15,7 @@
> >      You should have received a copy of the GNU Lesser General Public
> >      License along with the GNU C Library; if not, see
> >      <https://www.gnu.org/licenses/>.  */
> > -
> > +#include <stdio.h>
>
> This is most likely not required.

Yes. I forgot to remove that.

> >   #if defined __GNUC__ && !defined _SOFT_FLOAT && !defined __NO_FPRS__
> >   
> >   /* Inline definitions for fegetround.  */
> > @@ -51,9 +51,16 @@
> >   # define fegetround() __fegetround ()
> >   
> >   # ifndef __NO_MATH_INLINES
> > -/* 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'.  */
>
> This is an installed header which can potentially be used with a older
> gcc version. It is most likely an old bug fixed on newer release, so to
> use the plain 'i' constrain you need to set it to use iff with a gcc
> version where the warning surely does not happen.

Yes. I will check the minimal gcc version where the warning do not 
happen and set it. I am working on that here.

> > +
> > +/* Builtins to mtfsb0 and mtfsb1 was introduced on GCC 9.  */
> > +#  if !__GNUC_PREREQ(9, 0)
> > +
> > +#   define __builtin_mtfsb0(__b)					\
> > +   __asm__ __volatile__ ("mtfsb0 %0" : : "i" __b)
> > +
> > +#   define __builtin_mtfsb1(__b)					\
> > +   __asm__ __volatile__ ("mtfsb1 %0" : : "i" __b)
> > +#  endif
>
> I don't think it is a good practice to re-define bultins, use it along with
> an extra indirection instead.  Something as:
>
>    #  if !__GNUC_PREREQ(9, 0)
>    #   define __mtfsb0(__b) __asm__ __volatile__ ("mtfsb0 %0" : : "i" (__b))
>    #   define __mtfsb1(__b) __asm__ __volatile__ ("mtfsb1 %0" : : "i" (__b))
>    #  else
>    #   define __mtfsb0(__b) __builtin_mtfsb0 (__b)
>    #   define __mtfsb1(__b) __builtin_mtfsb1 (__b)
>    #  endif

Nice. I'll do that. Thank you for this suggestion.

> >   
> >   #  if __GNUC_PREREQ(3, 4)
> >   
> > @@ -63,12 +70,11 @@
> >       int __e = __excepts;						      \
> >       int __ret;								      \
> >       if (__builtin_constant_p (__e)					      \
> > -        && (__e & (__e - 1)) == 0					      \
> > +        && (__builtin_popcount (__e) == 1)				      \
>
> Why change this code? The idea is not emit any of the check for constant
> call with only one FE_* flag (either mtfsb0 or mtfsb1).

In that case the test (__e & (__e - 1)) == 0 tests if only one bit is 
set on __e. Since FE_* flags are in power of two (except FE_ALL_EXCEPT) 
it has the same effect that __builtin_popcount. I guess is more readable 
to use __builtin_popcount(__e) == 1. The code is still the same: not 
emit any of the check for constant call it only one FE_ flag is set.

> >           && __e != FE_INVALID)						      \
> >         {									      \
> >   	if (__e != 0)							      \
> > -	  __asm__ __volatile__ ("mtfsb1 %0"				      \
> > -				: : "i#*X" (__builtin_clz (__e)));	      \
> > +	  __builtin_mtfsb1 ((__builtin_clz (__e)));			      \
> >           __ret = 0;							      \
> >         }									      \
> >       else								      \
> > @@ -82,12 +88,11 @@
> >       int __e = __excepts;						      \
> >       int __ret;								      \
> >       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)));	      \
> > +	  __builtin_mtfsb0 ((__builtin_clz (__e)));			      \
> >           __ret = 0;							      \
> >         }									      \
> >       else								      \
> > --
> > 2.17.1