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

[tcwang]

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

[manojgupta]

clang/include/clang/Driver/Options.td

2218

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

2219

ARM/AArch64/PowerPC

2221

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]

Fix some comments.

[tcwang]

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.

[DavidSpickett]

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.

[peter.smith]

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

• Arm are trying to keep the options for controlling target features as consistent as possible with GCC and this option isn't supported in GCC (https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html)
• There is http://lists.llvm.org/pipermail/llvm-dev/2018-September/126346.html which is Arm's proposal for how target options can be better supported in Clang. I think that supporting -mcrypto as well would add more complexity as there are interactions between the options.
• Arm 32-bit also supports crypto so this would need to be added to Arm as well for consistency.

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

[manojgupta]

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.

[peter.smith]

In D60472#1461336, @manojgupta wrote:

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.

[manojgupta]

In D60472#1461351, @peter.smith wrote:

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.

[manojgupta]

In D60472#1461351, @peter.smith wrote:

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.

clang/include/clang/Driver/Options.td

 def mpower8_vector : Flag<["-"], "mpower8-vector">,
     Group<m_ppc_Features_Group>;
 def mno_power8_vector : Flag<["-"], "mno-power8-vector">,
     Group<m_ppc_Features_Group>;
 def mpower9_vector : Flag<["-"], "mpower9-vector">,
     Group<m_ppc_Features_Group>;
 def mno_power9_vector : Flag<["-"], "mno-power9-vector">,
     Group<m_ppc_Features_Group>;
-def mpower8_crypto : Flag<["-"], "mcrypto">,
-    Group<m_ppc_Features_Group>;
-def mnopower8_crypto : Flag<["-"], "mno-crypto">,
-    Group<m_ppc_Features_Group>;
+def mcrypto : Flag<["-"], "mcrypto">, Group<m_Group>,
+    HelpText<"Add use of cryptographic instructions (ARM/PowerPC only)">;
+def mnocrypto : Flag<["-"], "mno-crypto">, Group<m_Group>,
+    HelpText<"Disallow use of cryptographic instructions (ARM/PowerPC only)">;
 def mdirect_move : Flag<["-"], "mdirect-move">,
     Group<m_ppc_Features_Group>;
 def mnodirect_move : Flag<["-"], "mno-direct-move">,
     Group<m_ppc_Features_Group>;
 def mhtm : Flag<["-"], "mhtm">, Group<m_ppc_Features_Group>;
 def mno_htm : Flag<["-"], "mno-htm">, Group<m_ppc_Features_Group>;
 def mfprnd : Flag<["-"], "mfprnd">, Group<m_ppc_Features_Group>;
 def mno_fprnd : Flag<["-"], "mno-fprnd">, Group<m_ppc_Features_Group>;

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

void aarch64::getAArch64TargetFeatures(const Driver &D,
   else if (success &&
            (Args.hasArg(options::OPT_arch) || isCPUDeterminedByTriple(Triple)))
     success = getAArch64MicroArchFeaturesFromMcpu(
         D, getAArch64TargetCPU(Args, Triple, A), Args, Features);
 
   if (!success)
     D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 
+  // En/disable crypto
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto,
+                               options::OPT_mgeneral_regs_only)) {
+    if (A->getOption().matches(options::OPT_mcrypto))
+      Features.push_back("+crypto");
+    else
+      Features.push_back("-crypto");
+  }
+
   if (Args.getLastArg(options::OPT_mgeneral_regs_only)) {
     Features.push_back("-fp-armv8");
     Features.push_back("-crypto");
     Features.push_back("-neon");
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mtp_mode_EQ)) {
     StringRef Mtp = A->getValue();

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

fp16_fml_fallthrough:
   // En/disable crc code generation.
   if (Arg *A = Args.getLastArg(options::OPT_mcrc, options::OPT_mnocrc)) {
     if (A->getOption().matches(options::OPT_mcrc))
       Features.push_back("+crc");
     else
       Features.push_back("-crc");
   }
 
+  // En/disable crypto
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto)) {
+    if (A->getOption().matches(options::OPT_mcrypto) && ABI != arm::FloatABI::Soft)
+      Features.push_back("+crypto");
+    else
+      Features.push_back("-crypto");
+  }
+
   // For Arch >= ARMv8.0:  crypto = sha2 + aes
   // FIXME: this needs reimplementation after the TargetParser rewrite
   if (ArchName.find_lower("armv8a") != StringRef::npos ||
       ArchName.find_lower("armv8.1a") != StringRef::npos ||
       ArchName.find_lower("armv8.2a") != StringRef::npos ||
       ArchName.find_lower("armv8.3a") != StringRef::npos ||
       ArchName.find_lower("armv8.4a") != StringRef::npos) {
     if (ArchName.find_lower("+crypto") != StringRef::npos) {

clang/lib/Driver/ToolChains/Arch/PPC.h

 
 namespace clang {
 namespace driver {
 namespace tools {
 namespace ppc {
 
 bool hasPPCAbiArg(const llvm::opt::ArgList &Args, const char *Value);
 
+bool hasCryptoFeatureEnabled(const llvm::opt::ArgList &Args);
+
 enum class FloatABI {
   Invalid,
   Soft,
   Hard,
 };
 
 enum class ReadGOTPtrMode {
   Bss,

clang/lib/Driver/ToolChains/Arch/PPC.cpp

void ppc::getPPCTargetFeatures(const Driver &D, const llvm::Triple &Triple,
 
   ppc::FloatABI FloatABI = ppc::getPPCFloatABI(D, Args);
   if (FloatABI == ppc::FloatABI::Soft)
     Features.push_back("-hard-float");
 
   ppc::ReadGOTPtrMode ReadGOT = ppc::getPPCReadGOTPtrMode(D, Triple, Args);
   if (ReadGOT == ppc::ReadGOTPtrMode::SecurePlt)
     Features.push_back("+secure-plt");
+
+  if (ppc::hasCryptoFeatureEnabled(Args))
+    Features.push_back("+crypto");
 }
 
 ppc::ReadGOTPtrMode ppc::getPPCReadGOTPtrMode(const Driver &D, const llvm::Triple &Triple,
                                               const ArgList &Args) {
   if (Args.getLastArg(options::OPT_msecure_plt))
     return ppc::ReadGOTPtrMode::SecurePlt;
   if (Triple.isOSNetBSD() || Triple.isOSOpenBSD())
     return ppc::ReadGOTPtrMode::SecurePlt;
ppc::FloatABI ppc::getPPCFloatABI(const Driver &D, const ArgList &Args) {
 
   return ABI;
 }
 
 bool ppc::hasPPCAbiArg(const ArgList &Args, const char *Value) {
   Arg *A = Args.getLastArg(options::OPT_mabi_EQ);
   return A && (A->getValue() == StringRef(Value));
 }
+
+bool ppc::hasCryptoFeatureEnabled(const ArgList &Args) {
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto))
+    if (A->getOption().matches(options::OPT_mcrypto))
+      return true;
+  return false;
+}

clang/test/Driver/aarch64-crypto.c

+// REQUIRES: aarch64-registered-target
+// RUN: %clang -target aarch64 -mcrypto -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-AARCH64-CRYPTO < %t %s
+// CHECK-AARCH64-CRYPTO: "-target-feature" "+crypto"
+
+// RUN: %clang -target aarch64 -mno-crypto -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-AARCH64-NOCRYPTO < %t %s
+// CHECK-AARCH64-NOCRYPTO: "-target-feature" "-crypto"

clang/test/Driver/armv8-crypto.c

+// REQUIRES: arm-registered-target
+// RUN: %clang -target armv8 -mcrypto -mhard-float -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-V8-CRYPTO < %t %s
+// CHECK-V8-CRYPTO: "-target-feature" "+crypto"
+
+// RUN: %clang -target armv8 -mno-crypto -mhard-float -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-V8-NOCRYPTO < %t %s
+// CHECK-V8-NOCRYPTO: "-target-feature" "-crypto"