[Buildroot] [PATCH] package/Makefile.in: Tweak TARGET_ABI for MPC8548

[Buildroot] [PATCH] package/Makefile.in: Tweak TARGET_ABI for MPC8548

[Andrey Smirnov]

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
 endif
 ifeq ($(BR2_powerpc_e500mc),y)
 TARGET_ABI += -mabi=spe -mfloat-gprs=double -Wa,-me500mc
-- 
2.9.4

[Buildroot] [PATCH] package/Makefile.in: Tweak TARGET_ABI for MPC8548

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] package/Makefile.in: Tweak TARGET_ABI for MPC8548

[Andrey Smirnov]

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

[Buildroot] [PATCH] package/Makefile.in: Tweak TARGET_ABI for MPC8548

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] package/Makefile.in: Tweak TARGET_ABI for MPC8548

[Andrey Smirnov]

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