[ARM] Take into account -mcpu and -mfpu options while handling 'crypto' feature

[krisb]

Summary

'+crypto' means '+aes' and '+sha2' for arch >= ARMv8 when they were
not disabled explicitly. But this is correctly handled only in case of
'-march' option, though the feature may also be specified through
the '-mcpu' or '-mfpu' options. In the following example:

$ clang -mcpu=cortex-a57 -mfpu=crypto-neon-fp-armv8

'aes' and 'sha2' are disabled that is quite unexpected:

$ clang -cc1 -triple armv8--- -target-cpu cortex-a57
  <...> -target-feature -sha2 -target-feature -aes -target-feature +crypto

This exposed by https://reviews.llvm.org/D63936 that makes
the 'aes' and 'sha2' features disabled by default.

So, while handling the 'crypto' feature we need to take into account:

• a CPU name, as it provides the information about architecture (if no '-march' option specified),
• features, specified by the '-mcpu' and '-mfpu' options.

[dnsampaio]

lib/Driver/ToolChains/Arch/ARM.cpp

482–486 ↗

(On Diff #214387)

Could just use llvm::ARM::ArchKind arm::getLLVMArchKindForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple) ?

[krisb]

Applied the comment.

[krisb]

@dnsampaio, thanks, this looks better.

[dnsampaio]

Hi @krisb,
thanks for looking into this, and sorry for the delay, was out for a week.

lib/Driver/ToolChains/Arch/ARM.cpp

486–490 ↗

(On Diff #215394)

Could we do something a little more permanent here, for example it is already missing ARMV8_5A.
Digging I found the function StringRef arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple).
Perhaps just testing (llvm::ARM::parseArchVersion(arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple)) >= 8)
would do a better job.

659 ↗

(On Diff #215394)

Good catch.

[krisb]

Applied the comment & rebased.
Also changed the patch to take into account only the latest 'crypto' in the Features vector, to avoid enabling 'sha2' and 'aes' when 'crypto' was finally disabled.

[krisb]

@dnsampaio, thanks! This is definitely better. I also added ARMV8_5A as a test case.

[dnsampaio]

LGTM. One optional nit as it is not related with this patch anymore.

lib/Driver/ToolChains/Arch/ARM.cpp

659 ↗

(On Diff #215394)

For safety perhaps we can keep the CPU.empty() test.

[krisb]

Added 'CPU.empty()' check back.

[krisb]

@dnsampaio thanks for reviewing this! Could I ask you to commit the patch (since I don't have commit access yet)?

[dnsampaio]

clang -### -target arm-arm-none-eabit -march=armv8-m.main+crypto did not show +sha2 or +aes. After the patch it does.
I believe that is not expected, as in ARM.td crypto is not applied for any M profile. And Arm®v8-M Architecture Reference Manual does not reference these extensions neither.

lib/Driver/ToolChains/Arch/ARM.cpp

485 ↗

(On Diff #218076)

&& (llvm::ARM::parseArchProfile(Arch) == llvm::ARM::ProfileKind::A)

[krisb]

I see. But this doesn't seem like a valid case, does it? Shouldn't we somehow diagnose it to not to silently ignore user-specified options?

[dnsampaio]

Hi, I do agree that giving the user a warning that the argument is ignored is the best solution. If you wouldn't mind adding it to this patch, that would be great. Thanks.

[krisb]

Applied the comment: enable 'sha2' and 'aes' only for armv8 A profile, report warning and disable 'crypto' for other archs. Add more tests.

[dnsampaio]

LGTM. Thanks. Will commit for you as requested soon.

[krisb]

Rebased

[krisb]

@dnsampaio, many thanks for committing this!

cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td

def warn_target_unsupported_abslegacy : Warning<
   "ignoring '-mabs=legacy' option because the '%0' architecture does not support it">,
   InGroup<UnsupportedAbs>;
 def warn_target_unsupported_abs2008 : Warning<
   "ignoring '-mabs=2008' option because the '%0' architecture does not support it">,
   InGroup<UnsupportedAbs>;
 def warn_target_unsupported_compact_branches : Warning<
   "ignoring '-mcompact-branches=' option because the '%0' architecture does not"
   " support it">, InGroup<UnsupportedCB>;
+def warn_target_unsupported_extension : Warning<
+  "ignoring extension '%0' because the '%1' architecture does not support it">,
+   InGroup<InvalidCommandLineArgument>;
 def warn_drv_unsupported_gpopt : Warning<
   "ignoring '-mgpopt' option as it cannot be used with %select{|the implicit"
   " usage of }0-mabicalls">,
   InGroup<UnsupportedGPOpt>;
 def warn_drv_unsupported_longcalls : Warning<
   "ignoring '-mlong-calls' option as it is not currently supported with "
   "%select{|the implicit usage of }0-mabicalls">,
   InGroup<OptionIgnored>;

cfe/trunk/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");
   }
 
-  // For Arch >= ARMv8.0:  crypto = sha2 + aes
+  // For Arch >= ARMv8.0 && A profile:  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) {
-      if (ArchName.find_lower("+nosha2") == StringRef::npos)
+  auto CryptoIt =
+    llvm::find_if(llvm::reverse(Features),
+                  [](const StringRef F) { return F.contains("crypto"); });
+  if (CryptoIt != Features.rend() && CryptoIt->take_front() == "+") {
+    StringRef ArchSuffix = arm::getLLVMArchSuffixForARM(
+        arm::getARMTargetCPU(CPUName, ArchName, Triple), ArchName, Triple);
+    if (llvm::ARM::parseArchVersion(ArchSuffix) >= 8 &&
+        llvm::ARM::parseArchProfile(ArchSuffix) == llvm::ARM::ProfileKind::A) {
+      if (ArchName.find_lower("+nosha2") == StringRef::npos &&
+          CPUName.find_lower("+nosha2") == StringRef::npos)
         Features.push_back("+sha2");
-      if (ArchName.find_lower("+noaes") == StringRef::npos)
+      if (ArchName.find_lower("+noaes") == StringRef::npos &&
+          CPUName.find_lower("+noaes") == StringRef::npos)
         Features.push_back("+aes");
-    } else if (ArchName.find_lower("-crypto") != StringRef::npos) {
-      if (ArchName.find_lower("+sha2") == StringRef::npos)
-        Features.push_back("-sha2");
-      if (ArchName.find_lower("+aes") == StringRef::npos)
-        Features.push_back("-aes");
+    } else {
+      D.Diag(clang::diag::warn_target_unsupported_extension)
+        << "crypto" << llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
+      Features.push_back("-crypto");
     }
   }
 
   // CMSE: Check for target 8M (for -mcmse to be applicable) is performed later.
   if (Args.getLastArg(options::OPT_mcmse))
     Features.push_back("+8msecext");
 
   // Look for the last occurrence of -mlong-calls or -mno-long-calls. If

 
 /// getLLVMArchSuffixForARM - Get the LLVM ArchKind value to use for a
 /// particular CPU (or Arch, if CPU is generic). This is needed to
 /// pass to functions like llvm::ARM::getDefaultFPU which need an
 /// ArchKind as well as a CPU name.
 llvm::ARM::ArchKind arm::getLLVMArchKindForARM(StringRef CPU, StringRef Arch,
                                                const llvm::Triple &Triple) {
   llvm::ARM::ArchKind ArchKind;
-  if (CPU == "generic") {
+  if (CPU == "generic" || CPU.empty()) {
     std::string ARMArch = tools::arm::getARMArch(Arch, Triple);
     ArchKind = llvm::ARM::parseArch(ARMArch);
     if (ArchKind == llvm::ARM::ArchKind::INVALID)
       // In case of generic Arch, i.e. "arm",
       // extract arch from default cpu of the Triple
       ArchKind = llvm::ARM::parseCPUArch(Triple.getARMCPUForArch(ARMArch));
   } else {
     // FIXME: horrible hack to get around the fact that Cortex-A7 is only an

cfe/trunk/test/Driver/arm-features.c

 //
 // Check +crypto:
 //
 // RUN: %clang -target arm-arm-none-eabi -march=armv8a+crypto   -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2 %s
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.1a+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2 %s
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.2a+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2 %s
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.3a+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2 %s
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.4a+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2 %s
-// CHECK-CRYPTO2: "-cc1"{{.*}} "-target-cpu" "generic"{{.*}} "-target-feature" "+crypto" "-target-feature" "+sha2" "-target-feature" "+aes"
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.5a+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2 %s
+// CHECK-CRYPTO2: "-cc1"{{.*}} "-target-cpu" "generic"{{.*}} "-target-feature" "+crypto"{{.*}} "-target-feature" "+sha2" "-target-feature" "+aes"
 //
 // Check -crypto:
 //
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.1a+nocrypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO2 %s
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.2a+nocrypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO2 %s
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.3a+nocrypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO2 %s
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.4a+nocrypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO2 %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.5a+nocrypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO2 %s
 // CHECK-NOCRYPTO2-NOT: "-target-feature" "+crypto" "-target-feature" "+sha2" "-target-feature" "+aes"
 //
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2-CPU %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57 -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO2-CPU %s
+// CHECK-CRYPTO2-CPU: "-cc1"{{.*}} "-target-cpu" "cortex-a57"{{.*}} "-target-feature" "+crypto"{{.*}} "-target-feature" "+sha2" "-target-feature" "+aes"
+//
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57+norypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO2-CPU %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57 -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO2-CPU %s
+// CHECK-NOCRYPTO2-CPU-NOT: "-cc1"{{.*}} "-target-cpu" "cortex-a57"{{.*}} "-target-feature" "+crypto"{{.*}} "-target-feature" "+sha2" "-target-feature" "+aes"
+//
 // Check +crypto -sha2 -aes:
 //
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.1a+crypto+nosha2+noaes -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO3 %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57+crypto+nosha2+noaes -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO3 %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57+nosha2+noaes -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO3 %s
 // CHECK-CRYPTO3-NOT: "-target-feature" "+sha2" "-target-feature" "+aes"
 //
 // Check -crypto +sha2 +aes:
 //
 // RUN: %clang -target arm-arm-none-eabi -march=armv8.1a+nocrypto+sha2+aes -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO4 %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57+nocrypto+sha2+aes -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO4 %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-a57+sha2+aes -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-CRYPTO4 %s
 // CHECK-CRYPTO4: "-target-feature" "+sha2" "-target-feature" "+aes"
+//
+// Check +crypto for M and R profiles:
+//
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-r+crypto   -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-m.base+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv8-m.main+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
+// RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-m23+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
+// CHECK-NOCRYPTO5: warning: ignoring extension 'crypto' because the {{.*}} architecture does not support it
+// CHECK-NOCRYPTO5-NOT: "-target-feature" "+crypto"{{.*}} "-target-feature" "+sha2" "-target-feature" "+aes"