Page MenuHomePhabricator

[AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64
Needs ReviewPublic

Authored by tcwang on Apr 9 2019, 10:28 AM.

Details

Summary

This patch enhances Clang flags -mcrypto and -mno-crypto to enable/disable crypto feature.
Before this change, -m(no-)crypto is exclusively for Power8. We want to add this feature
to ARM/AArch64 as well. With -mcrypto is specified, '+crypto' will be passed as
a target feature to ARM/AArch64/Power8, and -mno-cypto will remove target feature '-crypto'.

Event Timeline

tcwang created this revision.Apr 9 2019, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2019, 10:28 AM
tcwang retitled this revision from [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto. to [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64.Apr 9 2019, 10:32 AM
manojgupta added inline comments.Apr 9 2019, 11:03 AM
clang/include/clang/Driver/Options.td
2221–2222

Can you move it out of ppc specific options area to more generic options location e.g. like hard-float?

2222

ARM/AArch64/PowerPC

2224

ARM/AArch64/PowerPC

clang/lib/Driver/ToolChains/Arch/AArch64.cpp
192

I believe this should be merged with the code for OPT_mgeneral_regs_only otherwise the next if statement for mgeneral-regs-only would force "-crypto" .

if (A->getOption().matches(OPT_mgeneral_regs_only)))
.. disable crypto, neon etc.
else if (A->getOption().matches(options::OPT_mcrypto))
enable crypto
...

Please also add tests when mgeneral-regs-only is specified with "-mcrypto" before/after.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
453

Please add a test for interaction with soft-float ABI option.

tcwang updated this revision to Diff 194371.Apr 9 2019, 11:29 AM
tcwang marked an inline comment as done.

Fix some comments.

tcwang marked 4 inline comments as done.Apr 9 2019, 11:29 AM
tcwang added inline comments.
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
192

Understand. My question of the logic is that if -mgeneral-regs-only comes before -mcrypto, do we want to set "-fp-armv8" "-neon" or not? In other words, if -mcrypto comes after -mgeneral-regs-only, does it only override '+crypto' or override all the three features? I'll change the logic and add tests after making sure what we really want to do.

tcwang marked an inline comment as not done.Apr 9 2019, 11:30 AM

What's the motivation for this change, are you working towards common flags for both platforms? The current way to select crypto on AArch64 is '-march=armv8.x-a+crypto/nocrypto'. I can see that would be an issue if Power PC doesn't support that syntax, or doesn't have a specific crypto extension.

I'm not in favour of adding AArch64 support to -mcrypto -mnocrypto for a few reasons:

Can you expand on why you need -m[no]crypto?

The motivation for this change is to make "crypto" setting an additive option e.g. like "-mavx" used in many media packages. Some packages in Chrome want to enable crypto conditionally for a few files to allow crypto feature to be used based on runtime cpu detection. They set "-march=armv8+crypto" flag but it gets overridden by the global "-march=armv8a" flag set by the build system in Chrome OS because the target cpu does not support crypto causing compile-time errors.
Ability to specify "-mcrypto" standalone makes it an additive option and ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" from "-march" so that they could be set independently. The current additive alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.

The motivation for this change is to make "crypto" setting an additive option e.g. like "-mavx" used in many media packages. Some packages in Chrome want to enable crypto conditionally for a few files to allow crypto feature to be used based on runtime cpu detection. They set "-march=armv8+crypto" flag but it gets overridden by the global "-march=armv8a" flag set by the build system in Chrome OS because the target cpu does not support crypto causing compile-time errors.
Ability to specify "-mcrypto" standalone makes it an additive option and ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" from "-march" so that they could be set independently. The current additive alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.

Is that not a limitation of the build system? I'd expect a package to be able to locally override a global default rather than vice-versa. Although crypto might be the area of concern here and there may be a conveniently named option for PPC, where does this stop? Crypto is not the only architectural extension, for example see https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To maintain a consistent interface we'd need command line options for all the extensions. May I encourage you to reply to the RFC on command line options that I mentioned earlier if it doesn't work for you? I think the extensions need to be considered as a whole and not just individually.

Is that not a limitation of the build system? I'd expect a package to be able to locally override a global default rather than vice-versa. Although crypto might be the area of concern here and there may be a conveniently named option for PPC, where does this stop? Crypto is not the only architectural extension, for example see https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To maintain a consistent interface we'd need command line options for all the extensions. May I encourage you to reply to the RFC on command line options that I mentioned earlier if it doesn't work for you? I think the extensions need to be considered as a whole and not just individually.

While it partly is a build system issue, another problem is enabling crypto via "-march" requires picking an architecture as well. So even if it could override the global default, it would also override the global "-march" as well. If the global "-march" was a better/higher option, it does not make sense to override it e.g. "-march=armv8-a+crypto" overriding "-march=armv8.3-a" is not helpful since it will disable 8.3 features.

Is that not a limitation of the build system? I'd expect a package to be able to locally override a global default rather than vice-versa. Although crypto might be the area of concern here and there may be a conveniently named option for PPC, where does this stop? Crypto is not the only architectural extension, for example see https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To maintain a consistent interface we'd need command line options for all the extensions. May I encourage you to reply to the RFC on command line options that I mentioned earlier if it doesn't work for you? I think the extensions need to be considered as a whole and not just individually.

I'll also read the RFC and respond.