Message ID | 20170712024737.21578-1-andrew.smirnov@gmail.com |
---|---|
State | Rejected |
Headers | show |
Hello, On Tue, 11 Jul 2017 19:47:37 -0700, Andrey Smirnov wrote: > From: Mark Hinds <mhinds@spaceflightindustries.com> > > Add -mcpu=8548 -mhard-float to TARGET_ABI so that hardware FP is used > to build all of the package in the system. > > Signed-off-by: Mark Hinds <zoronic@gmail.com> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > package/Makefile.in | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/Makefile.in b/package/Makefile.in > index b95e5e3..958d72c 100644 > --- a/package/Makefile.in > +++ b/package/Makefile.in > @@ -78,7 +78,7 @@ ifeq ($(BR2_powerpc_8540),y) > TARGET_ABI += -mabi=spe -mfloat-gprs=single -Wa,-me500 > endif > ifeq ($(BR2_powerpc_8548),y) > -TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500x2 > +TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500x2 -mcpu=8548 -mhard-float Adding the -mcpu is useless, because it's already passed thanks to BR2_GCC_TARGET_CPU, defined to 8548 when BR2_powerpc_8548=y in arch/Config.in.powerpc. Unconditionally passing -mhard-float here is wrong, because people may want to use soft-float, which can be selected using the BR2_POWERPC_SOFT_FLOAT option. When BR2_SOFT_FLOAT=y (which is selected by BR2_POWERPC_SOFT_FLOAT), we: * Pass --with-float=soft when building gcc (see package/gcc/gcc.mk). This solves the case of internal toolchain being used. * Add -msoft-float in the toolchain wrapper when an external toolchain is used (toolchain/toolchain-external/pkg-toolchain-external.mk). However, we indeed never explicitly pass --with-float=hard (when building gcc) or -mhard-float (for external toolchains). Could you describe more precisely the problem you're seeing? Are you using the internal toolchain backend, or an external toolchain? In the latter case, which external toolchain are you using? Best regards, Thomas
On Wed, Jul 12, 2017 at 2:32 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Tue, 11 Jul 2017 19:47:37 -0700, Andrey Smirnov wrote: >> From: Mark Hinds <mhinds@spaceflightindustries.com> >> >> Add -mcpu=8548 -mhard-float to TARGET_ABI so that hardware FP is used >> to build all of the package in the system. >> >> Signed-off-by: Mark Hinds <zoronic@gmail.com> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> --- >> package/Makefile.in | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/package/Makefile.in b/package/Makefile.in >> index b95e5e3..958d72c 100644 >> --- a/package/Makefile.in >> +++ b/package/Makefile.in >> @@ -78,7 +78,7 @@ ifeq ($(BR2_powerpc_8540),y) >> TARGET_ABI += -mabi=spe -mfloat-gprs=single -Wa,-me500 >> endif >> ifeq ($(BR2_powerpc_8548),y) >> -TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500x2 >> +TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500x2 -mcpu=8548 -mhard-float > > Adding the -mcpu is useless, because it's already passed thanks to > BR2_GCC_TARGET_CPU, defined to 8548 when BR2_powerpc_8548=y in > arch/Config.in.powerpc. > Oh! Missed that, my bad. > Unconditionally passing -mhard-float here is wrong, because people may > want to use soft-float, which can be selected using the > BR2_POWERPC_SOFT_FLOAT option. > Good point, haven't thought about that. > When BR2_SOFT_FLOAT=y (which is selected by BR2_POWERPC_SOFT_FLOAT), we: > > * Pass --with-float=soft when building gcc (see package/gcc/gcc.mk). > This solves the case of internal toolchain being used. > > * Add -msoft-float in the toolchain wrapper when an external toolchain > is used (toolchain/toolchain-external/pkg-toolchain-external.mk). > > However, we indeed never explicitly pass --with-float=hard (when > building gcc) or -mhard-float (for external toolchains). > > Could you describe more precisely the problem you're seeing? Are you > using the internal toolchain backend, or an external toolchain? In the > latter case, which external toolchain are you using? > It's been a while, since I investigated this patch, but I believe we were seeing performance improvements on heavily numerical application on our system with "-mhard-float" while using custom built crosstools-ng e500 toolchain. But given how I didn't notice two significant problems with this patch and it is clearly not ready for prime time, let me go back and do more research/double check that I wasn't doing something dumb and missing the obvious. I'll follow up with a separate patch if it's truly useful. Thanks, Andrey Smirnov
Hello, On Fri, 14 Jul 2017 14:47:15 -0500, Andrey Smirnov wrote: > It's been a while, since I investigated this patch, but I believe we > were seeing performance improvements on heavily numerical application > on our system with "-mhard-float" while using custom built > crosstools-ng e500 toolchain. But given how I didn't notice two > significant problems with this patch and it is clearly not ready for > prime time, let me go back and do more research/double check that I > wasn't doing something dumb and missing the obvious. > > I'll follow up with a separate patch if it's truly useful. I think it definitely makes sense to continue work on this patch. PowerPC is not the most well maintained architecture in Buildroot, so it's very good to get contributions from our PowerPC users to improve the support for this architecture. If indeed PowerPC needs an explicit -mhard-float to use the FPU, we'll have to fix Buildroot. Could you investigate this a little bit more? Thanks! Thomas
On Fri, Jul 14, 2017 at 3:59 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Fri, 14 Jul 2017 14:47:15 -0500, Andrey Smirnov wrote: > >> It's been a while, since I investigated this patch, but I believe we >> were seeing performance improvements on heavily numerical application >> on our system with "-mhard-float" while using custom built >> crosstools-ng e500 toolchain. But given how I didn't notice two >> significant problems with this patch and it is clearly not ready for >> prime time, let me go back and do more research/double check that I >> wasn't doing something dumb and missing the obvious. >> >> I'll follow up with a separate patch if it's truly useful. > > I think it definitely makes sense to continue work on this patch. > PowerPC is not the most well maintained architecture in Buildroot, so > it's very good to get contributions from our PowerPC users to improve > the support for this architecture. > > If indeed PowerPC needs an explicit -mhard-float to use the FPU, we'll > have to fix Buildroot. Could you investigate this a little bit more? > Absolutely. Sorry for not being very clear, what I was trying to say is: I just lost a bit of confidence that this patch is truly needed(I've been confuse by simpler things before), so I'll go back and investigate more to either build a better case for it or discover what I missed. Thanks, Andrey Smirnov
diff --git a/package/Makefile.in b/package/Makefile.in index b95e5e3..958d72c 100644 --- a/package/Makefile.in +++ b/package/Makefile.in @@ -78,7 +78,7 @@ ifeq ($(BR2_powerpc_8540),y) TARGET_ABI += -mabi=spe -mfloat-gprs=single -Wa,-me500 endif ifeq ($(BR2_powerpc_8548),y) -TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500x2 +TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500x2 -mcpu=8548 -mhard-float endif ifeq ($(BR2_powerpc_e500mc),y) TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500mc