[OE-core] [PATCH] ARMv8-A: Add tune for AArch32 state and AArch64 state

[Andre McCurdy]

On Wed, Mar 30, 2016 at 9:36 AM, Khem Raj <raj.khem at gmail.com> wrote:
>
>> On Mar 30, 2016, at 9:03 AM, Dragomir Daniel <daniel.dragomir at windriver.com> wrote:
>>
>> On 03/29/2016 07:07 PM, Khem Raj wrote:
>>>> On Mar 28, 2016, at 3:10 PM, Phil Blundell <pb at pbcl.net> wrote:
>>>>
>>>> On Mon, 2016-03-28 at 18:29 +0300, Daniel Dragomir wrote:
>>>>> For AArch64 state, the default tune is aarch32 and include which
>>>>>    include just the aarch64 feature.
>>>> I don't really understand what this sentence is trying to say.  Can you
>>>> re-phrase it so as to be more accessible to non-experts?
>>>>
>>>>> meta/conf/machine/include/arm/arch-arm64.inc       |  36 -------
>>>>> meta/conf/machine/include/arm/arch-armv8.inc       |   1 -
>>>> If you are deleting existing files, please mention that in the commit
>>>> message.  And in particular, if you are removing a file that supports
>>>> generic ARMv8 (if imperfectly) and replacing it with something that is
>>>> specific to ARMv8-A, please explain why this is a good thing.
>>>>
>>>>> +TUNEVALID[crc] = "Enable CRC instructions for ARMv8-a"
>>>>> +ARMPKGSFX_FPU .= "${@bb.utils.contains('TUNE_FEATURES', 'crc', '-crc', '', d)}"
>>>>> +ARMPKGSFX_FPU_64 = "${@bb.utils.contains('TUNE_FEATURES', 'crc', '-crc', '', d)}"
>>>> Why is this involved with ARMPKGSFX_FPU?  The crc instructions are not
>>>> related to the fpu.  Also, the fact that you need to duplicate both this
>>>> and the crypto one for both FPU and FPU_64 seems like an indication that
>>>> something elsewhere is misdesigned.
>>>>
>>>>> +TUNEVALID[fp-armv8] = "Enable ARMv8 Vector Floating Point unit."
>>>>> +ARMPKGSFX_FPU .= "${@bb.utils.contains('TUNE_FEATURES', 'fp-armv8', '-fp-armv8', '', d)}"
>>>> I don't entirely understand why this one doesn't have an FPU_64
>>>> equivalent.  Are you always enabling vfp for A64?
>>>>
>>>>> +MACHINEOVERRIDES =. "${@bb.utils.contains('TUNE_FEATURES', 'aarch32', 'aarch32:', '' ,d)}"
>>>>> +MACHINEOVERRIDES .= "${@bb.utils.contains('TUNE_FEATURES', 'aarch64', ':aarch64', '' ,d)}"
>>>> As previously discussed, I am not wild about "aarch32" as the name of an
>>>> override which really means "ARMv8 in AArch32 state".  I know there is a
>>>> school of thought that says that the execution state of older cpus is
>>>> not AArch32 even if in practice it is indistinguishable, but this
>>>> doesn't seem to match either the expectations that a rational user of OE
>>>> is likely to have, or the information in the published literature.
>>> Don’t disagree with technical argument I said that previously too. All I am trying
>>> to say is lets take this opportunity to simplify arm tunes starting with armv8
>>> we have this opportunity and what you suggest will keep the thread alive with
>>> prior tunes. With armv8 we should match what we do for x86 and x86_64 ideally.
>>
>> We'll need to take a decision on this and continue the work on tunes.
>>
>> What is the best way to name the armv8a tunes:
>> aarch32 and aarch64 or armv8a32 and armv8a64?
>
> We are catering armv8 here and not 64bit or 32bit arm. Thats the difference.
> may be those could be added in a common file?
> its fine to call them armv8a32 and armv8a64 it doesnt matter
> but break the inclusion of .inc files that will drag it
> into pre-armv8 world and define a consistent 32bit tunes
> such that when we think armv8, we get three options
> aarch64, aarch32, aarch32t2,

All armv8a CPUs support Thumb2, so there's no need to distinguish
between aarch32 and aarch32t2.

We've made that mistake historically for armv6 and armv7 but we
shouldn't keep carrying it forward.

> all the vfp options
> may be folder into these three and also neon and others

For most cores from Cortex A9 onwards NEON and VFP are optional, so
hardcoded assumptions won't work.

> Can we reach that somehow ?
>
>
>>
>> Who else need to express his opinion about this and make the decision?
>>
>> Daniel
>>
>>>
>>>>> +ARMPKGARCH_tune-aarch64            ?= "aarch64"
>>>> This seems a tiny bit short-sighted since presumably there will be an
>>>> ARMv9 (or ARMv8.2) at some point.  What will ARMPKGARCH for A64 be then?
>>>>
>>>>> +BASE_LIB_tune-aarch64            = "lib64"
>>>> This seems like more a matter of distro policy and I'm not sure it
>>>> belongs in a tune file.  If it does then can't you rely on the aarch64
>>>> MACHINEOVERRIDE and just write "BASE_LIB_aarch64" rather than having to
>>>> duplicate this for all four of the tunes (and the -be equivalents)?
>>>>
>>>>> +# Duplicated from arch-arm.inc
>>>> Please add a comment explaining why you can't include arch-arm.inc.
>>>>
>>>> p.
>>>>
>>>>
>>>> --
>>>> _______________________________________________
>>>> Openembedded-core mailing list
>>>> Openembedded-core at lists.openembedded.org
>>>> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>>
>
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core at lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>