[ARM] Preserve fpu behaviour for '-crypto'

[dnsampaio]

Summary

This patch restores the behaviour that -fpu overwrites the
architecture obtained from -march or -mcpu flags, not enforcing to
disable 'crypto' if march=armv7 and mfpu=neon-fp-armv8.
However, it does warn that 'crypto' is ignored when passing
mfpu=crypto-neon-fp-armv8.

[peter.smith]

After retesting on a failing example and experimenting a bit, I think that we should only preserve +crypto with -fno-integrated-as. It would also be good to mention D66018 and maybe add the original author as a reviewer.

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

482–508

Looking into this in more detail I think the comment can be made more specific. It seems like the main reason to keep +crypto is that when the non-integrated-assembler is used, then we get the directive:

.fpu crypto-neon-fp-armv8

In arm-none-eabi-as the assembler will support crypto instructions. Clang will not as any use of crypto instructions will fail due lack of v8 support.

With -fno-integrated-as -mfpu=crypto-neon-fp-armv8 some assemblers such as the GNU assembler will permit the use of crypto instructions as the fpu will override the architecture. We keep the crypto feature in this case to preserve compatibility. In all other cases we remove the crypto feature.

485

I think we should only do this for -fno-integrated-as as the integrated assembler will fail if give a crypto instruction even with this change. With the integrated assembler we get:

error: instruction requires: armv8
        aese.8 q8, q9

clang/test/Driver/arm-features.c

84

arm-arm-none-eabi is a vendor specific triple? Better to use arm-none-eabi

[dnsampaio]

Attending review request:

• Fixed to only add '-crypto' when not passing -fno-integrated-as
• Fixed test to use arm-none-none-eabi
• Changed comments

[peter.smith]

Thanks for the update. That looks good to me. Will be good to wait for a day before committing to give US timezone a chance to object.

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

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 && A profile:  crypto = sha2 + aes
   // FIXME: this needs reimplementation after the TargetParser rewrite
-  auto CryptoIt =
-    llvm::find_if(llvm::reverse(Features),
-                  [](const StringRef F) { return F.contains("crypto"); });
-  if (CryptoIt != Features.rend() && CryptoIt->take_front() == "+") {
+  auto CryptoIt = llvm::find_if(llvm::reverse(Features), [](const StringRef F) {
+    return F.contains("crypto");
+  });
+  if (CryptoIt != Features.rend()) {
+    if (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) {
+          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 &&
             CPUName.find_lower("+noaes") == StringRef::npos)
           Features.push_back("+aes");
       } else {
         D.Diag(clang::diag::warn_target_unsupported_extension)
-        << "crypto" << llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
+            << "crypto"
+            << llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
+        // With -fno-integrated-as -mfpu=crypto-neon-fp-armv8 some assemblers such as the GNU assembler
+        // will permit the use of crypto instructions as the fpu will override the architecture.
+        // We keep the crypto feature in this case to preserve compatibility.
+        // In all other cases we remove the crypto feature.
+        if (!Args.hasArg(options::OPT_fno_integrated_as))
           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
   // neither options are specified, see if we are compiling for kernel/kext and
   // decide whether to pass "+long-calls" based on the OS and its version.

clang/test/Driver/arm-features.c

 // 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"
+//
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but it does warn that +crypto has no effect
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+aes -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+sha2 -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+sha2+aes -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+aes -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+sha2 -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+sha2+aes -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=ALL,CHECK-HASSHA,CHECK-HASAES %s
+// CHECK-WARNONLY: warning: ignoring extension 'crypto' because the 'armv7-a' architecture does not support it
+// ALL:     "-target-feature"
+// CHECK-WARNONLY-NOT: "-target-feature" "-crypto"
+// CHECK-HASSHA-SAME:  "-target-feature" "+sha2"
+// CHECK-HASAES-SAME:  "-target-feature" "+aes"
+//