Patchwork [CONSOLIDATED,PULL,03/19] tune: Add hard floating point variants of cortexa8 tunes

login
register
mail settings
Submitter Saul Wold
Date Aug. 22, 2011, 9:51 p.m.
Message ID <9367246f3304f18ffb82a2b588337a073d830b51.1314043945.git.sgw@linux.intel.com>
Download mbox | patch
Permalink /patch/10407/
State New, archived
Headers show

Comments

Saul Wold - Aug. 22, 2011, 9:51 p.m.
From: Darren Hart <dvhart@linux.intel.com>

Enable machines or distros to select the hard floating point abi for cortexa8
machines. I left out the arm7a thumb+neon combinations as they were not
present in the original non-hf set.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Jason Kridner <jkridner@beagleboard.org>
CC: Koen Kooi <koen@dominion.thruhere.net>
---
 meta/conf/machine/include/tune-cortexa8.inc |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)
Phil Blundell - Aug. 23, 2011, 9:30 a.m.
On Mon, 2011-08-22 at 14:51 -0700, Saul Wold wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> 
> Enable machines or distros to select the hard floating point abi for cortexa8
> machines. I left out the arm7a thumb+neon combinations as they were not
> present in the original non-hf set.
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> CC: Jason Kridner <jkridner@beagleboard.org>
> CC: Koen Kooi <koen@dominion.thruhere.net>
> ---
>  meta/conf/machine/include/tune-cortexa8.inc |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/conf/machine/include/tune-cortexa8.inc b/meta/conf/machine/include/tune-cortexa8.inc
> index 67c5f0b..edd1717 100644
> --- a/meta/conf/machine/include/tune-cortexa8.inc
> +++ b/meta/conf/machine/include/tune-cortexa8.inc
> @@ -5,12 +5,22 @@ require conf/machine/include/arm/arch-armv7a.inc
>  TUNEVALID[cortexa8] = "Enable Cortex-A8 specific processor optimizations"
>  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "cortexa8", "-mtune=cortex-a8", "", d)}"
>  
> -AVAILTUNES += "cortexa8 cortexa8t"
> +# Little Endian base configs
> +AVAILTUNES += "cortexa8 cortexa8t cortexa8-neon"
>  TUNE_FEATURES_tune-cortexa8 = "${TUNE_FEATURES_tune-armv7a} cortexa8"
>  TUNE_FEATURES_tune-cortexa8t = "${TUNE_FEATURES_tune-armv7at} cortexa8"
> -TUNE_FEATURES_tune-cortexa8-neon = "${TUNE_FEATURES_tune-cortexa8} neon"
> -
> +TUNE_FEATURES_tune-cortexa8-neon = "${TUNE_FEATURES_tune-armv7a-neon} cortexa8"
>  PACKAGE_EXTRA_ARCHS_tune-cortexa8 = "${PACKAGE_EXTRA_ARCHS_tune-armv7a}"
>  PACKAGE_EXTRA_ARCHS_tune-cortexa8t = "${PACKAGE_EXTRA_ARCHS_tune-armv7at}"
>  PACKAGE_EXTRA_ARCHS_tune-cortexa8-neon = "${PACKAGE_EXTRA_ARCHS_tune-armv7a-neon}"

This part of the patch doesn't seem to match any of the description in
the checkin comment.

> +# VFP Tunes
> +AVAILTUNES += "cortexa8hf cortexa8thf cortexa8hf-neon"
> +TUNE_FEATURES_tune-cortexa8hf ?= "${TUNE_FEATURES_tune-armv7ahf} cortexa8"
> +TUNE_FEATURES_tune-cortexa8thf ?= "${TUNE_FEATURES_tune-armv7athf} cortexa8"
> +TUNE_FEATURES_tune-cortexa8hf-neon ?= "${TUNE_FEATURES_tune-armv7ahf-neon} cortexa8"
> +PACKAGE_EXTRA_ARCHS_tune-cortexa8hf = "${PACKAGE_EXTRA_ARCHS_tune-armv7ahf}"
> +PACKAGE_EXTRA_ARCHS_tune-cortexa8thf = "${PACKAGE_EXTRA_ARCHS_tune-armv7athf}"
> +PACKAGE_EXTRA_ARCHS_tune-cortexa8hf-neon = "${PACKAGE_EXTRA_ARCHS_tune-armv7ahf-neon}"

I don't think "VFP Tunes" is a very good description of these, since it
might lead folks to think that they need to enable these tunes to get
vfp instructions (which is untrue: the vfp ISA is controlled by the
"vfp" tune feature and this is set for both armv6 and armv7a
automatically).  If they are hard-float ABI tunes, which seems to be the
case, then let's call them that. 

Also, on a broader issue, I continue to feel that adding more and more
terms to what is already quite a large cross product is not a very good
way to proceed.  There's nothing very cortexa8 specific about the
hardfloat API and, to do this comprehensively, we'd have to add an -hf
version of every existing tune definition which has vfp enabled.  I
think it would be better to find a way to express this sort of thing
which captures the orthogonality of the different options and avoids the
need to write out every permutation in longhand.

Also also, on a more tangential note, I remain skeptical that the
hardfloat ABI is actually a useful thing to support.  I'd be interested
to see any benchmark results which demonstrate that it's worthwhile.

p.
Darren Hart - Aug. 23, 2011, 6:19 p.m.
On 08/23/2011 02:30 AM, Phil Blundell wrote:
> On Mon, 2011-08-22 at 14:51 -0700, Saul Wold wrote:
>> From: Darren Hart <dvhart@linux.intel.com>
>>
>> Enable machines or distros to select the hard floating point abi for cortexa8
>> machines. I left out the arm7a thumb+neon combinations as they were not
>> present in the original non-hf set.
>>
>> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
>> CC: Jason Kridner <jkridner@beagleboard.org>
>> CC: Koen Kooi <koen@dominion.thruhere.net>
>> ---
>>  meta/conf/machine/include/tune-cortexa8.inc |   16 +++++++++++++---
>>  1 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/meta/conf/machine/include/tune-cortexa8.inc b/meta/conf/machine/include/tune-cortexa8.inc
>> index 67c5f0b..edd1717 100644
>> --- a/meta/conf/machine/include/tune-cortexa8.inc
>> +++ b/meta/conf/machine/include/tune-cortexa8.inc
>> @@ -5,12 +5,22 @@ require conf/machine/include/arm/arch-armv7a.inc
>>  TUNEVALID[cortexa8] = "Enable Cortex-A8 specific processor optimizations"
>>  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "cortexa8", "-mtune=cortex-a8", "", d)}"
>>  
>> -AVAILTUNES += "cortexa8 cortexa8t"
>> +# Little Endian base configs
>> +AVAILTUNES += "cortexa8 cortexa8t cortexa8-neon"
>>  TUNE_FEATURES_tune-cortexa8 = "${TUNE_FEATURES_tune-armv7a} cortexa8"
>>  TUNE_FEATURES_tune-cortexa8t = "${TUNE_FEATURES_tune-armv7at} cortexa8"
>> -TUNE_FEATURES_tune-cortexa8-neon = "${TUNE_FEATURES_tune-cortexa8} neon"
>> -
>> +TUNE_FEATURES_tune-cortexa8-neon = "${TUNE_FEATURES_tune-armv7a-neon} cortexa8"
>>  PACKAGE_EXTRA_ARCHS_tune-cortexa8 = "${PACKAGE_EXTRA_ARCHS_tune-armv7a}"
>>  PACKAGE_EXTRA_ARCHS_tune-cortexa8t = "${PACKAGE_EXTRA_ARCHS_tune-armv7at}"
>>  PACKAGE_EXTRA_ARCHS_tune-cortexa8-neon = "${PACKAGE_EXTRA_ARCHS_tune-armv7a-neon}"
> 
> This part of the patch doesn't seem to match any of the description in
> the checkin comment.

True, this one was a change to make the assignments parallel to
each-other. I could submit this as a separate patch if people feel
strongly on the subject.

> 
>> +# VFP Tunes
>> +AVAILTUNES += "cortexa8hf cortexa8thf cortexa8hf-neon"
>> +TUNE_FEATURES_tune-cortexa8hf ?= "${TUNE_FEATURES_tune-armv7ahf} cortexa8"
>> +TUNE_FEATURES_tune-cortexa8thf ?= "${TUNE_FEATURES_tune-armv7athf} cortexa8"
>> +TUNE_FEATURES_tune-cortexa8hf-neon ?= "${TUNE_FEATURES_tune-armv7ahf-neon} cortexa8"
>> +PACKAGE_EXTRA_ARCHS_tune-cortexa8hf = "${PACKAGE_EXTRA_ARCHS_tune-armv7ahf}"
>> +PACKAGE_EXTRA_ARCHS_tune-cortexa8thf = "${PACKAGE_EXTRA_ARCHS_tune-armv7athf}"
>> +PACKAGE_EXTRA_ARCHS_tune-cortexa8hf-neon = "${PACKAGE_EXTRA_ARCHS_tune-armv7ahf-neon}"
> 
> I don't think "VFP Tunes" is a very good description of these, since it
> might lead folks to think that they need to enable these tunes to get
> vfp instructions (which is untrue: the vfp ISA is controlled by the
> "vfp" tune feature and this is set for both armv6 and armv7a
> automatically).  If they are hard-float ABI tunes, which seems to be the
> case, then let's call them that. 

I just copied this labeling across from the arm7a tune file.
Consistently wrong is still wrong however :-)

> 
> Also, on a broader issue, I continue to feel that adding more and more
> terms to what is already quite a large cross product is not a very good
> way to proceed.  There's nothing very cortexa8 specific about the
> hardfloat API and, to do this comprehensively, we'd have to add an -hf
> version of every existing tune definition which has vfp enabled.  I
> think it would be better to find a way to express this sort of thing
> which captures the orthogonality of the different options and avoids the
> need to write out every permutation in longhand.

I had a similar thought while writing it up. For the current release
cycle, I won't have time to try to come up with a new way to express
these. This approach is consistent with the existing tune descriptions.
Can we agree that a better method is needed while allowing this in to
get this support into testers hands?

> Also also, on a more tangential note, I remain skeptical that the
> hardfloat ABI is actually a useful thing to support.  I'd be interested
> to see any benchmark results which demonstrate that it's worthwhile.

I'm not sold on it either, but this lowers the barrier to people testing
with hardfp. I think having the ability to enable it is a good thing,
even if it is disabled by default.

I will respin this series once we come to a consensus on the concerns
Khem and Phil have raised.
Phil Blundell - Aug. 24, 2011, 10:46 a.m.
On Tue, 2011-08-23 at 11:19 -0700, Darren Hart wrote:
> I will respin this series once we come to a consensus on the concerns
> Khem and Phil have raised.

Thanks.  Looks like Richard merged it anyway, though, so probably no
need to respin. 

p.

Patch

diff --git a/meta/conf/machine/include/tune-cortexa8.inc b/meta/conf/machine/include/tune-cortexa8.inc
index 67c5f0b..edd1717 100644
--- a/meta/conf/machine/include/tune-cortexa8.inc
+++ b/meta/conf/machine/include/tune-cortexa8.inc
@@ -5,12 +5,22 @@  require conf/machine/include/arm/arch-armv7a.inc
 TUNEVALID[cortexa8] = "Enable Cortex-A8 specific processor optimizations"
 TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "cortexa8", "-mtune=cortex-a8", "", d)}"
 
-AVAILTUNES += "cortexa8 cortexa8t"
+# Little Endian base configs
+AVAILTUNES += "cortexa8 cortexa8t cortexa8-neon"
 TUNE_FEATURES_tune-cortexa8 = "${TUNE_FEATURES_tune-armv7a} cortexa8"
 TUNE_FEATURES_tune-cortexa8t = "${TUNE_FEATURES_tune-armv7at} cortexa8"
-TUNE_FEATURES_tune-cortexa8-neon = "${TUNE_FEATURES_tune-cortexa8} neon"
-
+TUNE_FEATURES_tune-cortexa8-neon = "${TUNE_FEATURES_tune-armv7a-neon} cortexa8"
 PACKAGE_EXTRA_ARCHS_tune-cortexa8 = "${PACKAGE_EXTRA_ARCHS_tune-armv7a}"
 PACKAGE_EXTRA_ARCHS_tune-cortexa8t = "${PACKAGE_EXTRA_ARCHS_tune-armv7at}"
 PACKAGE_EXTRA_ARCHS_tune-cortexa8-neon = "${PACKAGE_EXTRA_ARCHS_tune-armv7a-neon}"
 
+# VFP Tunes
+AVAILTUNES += "cortexa8hf cortexa8thf cortexa8hf-neon"
+TUNE_FEATURES_tune-cortexa8hf ?= "${TUNE_FEATURES_tune-armv7ahf} cortexa8"
+TUNE_FEATURES_tune-cortexa8thf ?= "${TUNE_FEATURES_tune-armv7athf} cortexa8"
+TUNE_FEATURES_tune-cortexa8hf-neon ?= "${TUNE_FEATURES_tune-armv7ahf-neon} cortexa8"
+PACKAGE_EXTRA_ARCHS_tune-cortexa8hf = "${PACKAGE_EXTRA_ARCHS_tune-armv7ahf}"
+PACKAGE_EXTRA_ARCHS_tune-cortexa8thf = "${PACKAGE_EXTRA_ARCHS_tune-armv7athf}"
+PACKAGE_EXTRA_ARCHS_tune-cortexa8hf-neon = "${PACKAGE_EXTRA_ARCHS_tune-armv7ahf-neon}"
+
+