Message ID | 12b03cc3-0a23-ed07-e12d-8bdb6c7da9cb@linux.ibm.com |
---|---|
State | Superseded |
Delegated to: | Adhemerval Zanella Netto |
Headers |
Received: (qmail 51793 invoked by alias); 23 Jan 2020 18:14:21 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 51777 invoked by uid 89); 23 Jan 2020 18:14:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx0a-001b2d01.pphosted.com To: libc-alpha@sourceware.org From: Rogerio Alves <rcardoso@linux.ibm.com> Subject: [PATCH] powerpc: fenvinline.h refactor Message-ID: <12b03cc3-0a23-ed07-e12d-8bdb6c7da9cb@linux.ibm.com> Date: Thu, 23 Jan 2020 15:14:10 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------FA10501C2FDEF4408F924609" |
Commit Message
Rogerio Alves
Jan. 23, 2020, 6:14 p.m. UTC
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. Regard
Comments
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. > > 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. > --- > 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. > #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. > + > +/* 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 > > # 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). > && __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
* Adhemerval Zanella: > 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 I think __mtfsb0 and __mtfsb1 conflict with gcc's installed pu_intrinsics.h header. Thanks, Florian
On 24/01/2020 06:50, Florian Weimer wrote: > * Adhemerval Zanella: > >> 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 > > I think __mtfsb0 and __mtfsb1 conflict with gcc's installed > pu_intrinsics.h header. Right, in this case prepend with __glibc or any other related unique identifier. Checking on ppu_intrinsics.h, I wonder if the asm inline should not use the 'n' constraint as well.
> 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
On 27/01/2020 11:05, Rogerio Alves wrote: > >> 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. Also, __mtfsb{0,1} definition clash with ppu_intrinsics.h (which is an installed gcc header). So use __glibc_mtfsb{0,1} instead. > >>> # 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. My understanding it builds on the assumption that all the FE_* flags are multiple of 2, meaning only on bit will be set and thus if the exception to be set is not an multiple of 2 then it can not be set/unset using mtfsb{0,1}. This change is essentially the same, so I don't see a strong motivation to do it. > >>> && __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
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. --- 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> #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'. */ + +/* 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 # 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) \ && __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