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

[Darren Hart <dvhart@linux.intel.com>]

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.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel