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

[Dragomir Daniel <daniel.dragomir@windriver.com>]

[-- Attachment #1: Type: text/plain, Size: 5654 bytes --]

Thanks, Phil!
See my answers in-line.

Khem, Richard, Mark, can you please review and comment on this new
version of the patch?

On 03/29/2016 01:10 AM, Phil Blundell 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?

Sorry.

For AArch64 state, the default tune is aarch32 and include just the aarch64 feature.


>>   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.

OK, I'll add a comment.
Before, arch-armv8.inc which apparently add support for generic armv8, only
include arch-arm64.inc, so it's for 64-bit.
But just armv8-A supports 32 and 64 states... armv8-R and armv8-M are just
for 32. So it wasn't generic. Details here:
http://www.arm.com/products/processors/instruction-set-architectures/armv8-r-architecture.php

arch-arm8a.inc add now support for armv8-A processors which support 2 
states:
32-bit and 64-bit states. It makes more sense to have support for both 
states
in the same inc file because both refer to the same arch.

>
>> +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.

You're right. I'll add a new suffix variable for crc and crypto witch will
be use for both 32 and 64 tunes.

ARMPKGSFX_EXT - This is the EXTENSIONS specific suffix. The suffix
indicates specific extentions enabled. 'crc' and 'crypto' are defined.
Curently it is used just for armv8a architecture.

>
>> +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?

Yes. Floating point is enabled by default. From documentation:
"fp- Enable floating-point instructions. This is on by default for all 
possible values for options -marchand -mcpu."
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html

>
>> +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.

I changed armv8a with aarch32 because Khem Raj asked for a previous version
of the patch and in uther discussions related...
http://lists.openembedded.org/pipermail/openembedded-core/2016-March/118904.html

So, you say that we should not use aarch32 and aarch64 for armv8a, but
to use something like armv8a64 and armv8a32?
I have no problem with this, but we need to take a decision agreed by 
everyone.
It's the second time I change the tune names :)

>> +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?

Maybe with armv8a64 and armv8a32 this will be solved.

>
>> +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)?

BASE_LIB is defined also in many other tune files without any overwrites.
It was in aarch64.inc and I simply copied here.

It need to be used with suffix... see in Readme:
BASE_LIB_tune-<tune> - The "/lib" location for a specific ABI.  This is
used in a multilib configuration to place the libraries in the correct,
non-conflicting locations.

And this is how it's used in multilib.conf
baselib = "${@d.getVar('BASE_LIB_tune-' + (d.getVar('DEFAULTTUNE', True) or 'INVALID'), True) or d.getVar('BASELIB', True)}"

I can use ${BASE_LIB_tune-aarch64} for all the other tunes to be easier
to override.

>> +# Duplicated from arch-arm.inc
> Please add a comment explaining why you can't include arch-arm.inc.

I took the content from arch-arm64.inc and add it here. This duplication
was done in the first commit that add the arch-arm64.inc file.
So, I don't know exactly the reason. Maybe this was discussed on the first
implementation of aarch64 tune made by Mark.

Daniel

> p.
>
>


[-- Attachment #2: Type: text/html, Size: 10123 bytes --]