[AArch64] Make -march and target("arch=..") attributes imply dependent features

[dmgreen]

Summary

Specifying an architecture revision should also add feature strings for any dependent default extensions. Otherwise the new checks for target-dependent features for acle intrinsics from D134353 and D132034 can fail.

This patch does that in setFeatureEnabled, similar to the addition of dependent architecture revisions. +sve also needs to be added to armv9 architectures in the target parser, as it is implied by +sve2.

Fixes #59911

[danielkiss]

jut a NIT, LGTM otherwise.

clang/lib/Basic/Targets/AArch64.cpp

748

what do you think?

[lenary]

Can we check this logic, especially around adding both AEK_SVE and AEK_SVE2 in the AARCH64_ARCH descriptions, this means that -march=armv9-a+nosve2 still can generate sve instructions. I'm not entirely sure of the intended behaviour there, especially as sve2 should maybe imply sve by other means (iirc, there's a place higher up in AArch64TargetParser.def with the implied features?)

[dmgreen]

In D141411#4042891, @lenary wrote:

Can we check this logic, especially around adding both AEK_SVE and AEK_SVE2 in the AARCH64_ARCH descriptions, this means that -march=armv9-a+nosve2 still can generate sve instructions. I'm not entirely sure of the intended behaviour there, especially as sve2 should maybe imply sve by other means (iirc, there's a place higher up in AArch64TargetParser.def with the implied features?)

Hello. I would expect -march=armv9-a+nosve2 to still enable sve, and as far as I can tell (correct me if I'm wrong!) that is how this worked both before and after this patch. There is some code in the driver that already adds +sve and +sve2 TargetFeatures if the architecture is v9a. A lot of that code in the Driver shouldn't really be in their though, as it won't apply to target("arch=..") attributes correctly.

[dmgreen]

It does look like this might be enabling Crypto in places it was not in the past though. I'll look into why that is.

clang/lib/Basic/Targets/AArch64.cpp

return llvm::StringSwitch<bool>(Feature)
       .Cases("ls64", "ls64_v", "ls64_accdata", HasLS64)
       .Case("wfxt", HasWFxT)
       .Default(false);
 }
 
 void AArch64TargetInfo::setFeatureEnabled(llvm::StringMap<bool> &Features,
                                           StringRef Name, bool Enabled) const {
   Features[Name] = Enabled;
+
+  // If the feature is an architecture feature (like v8.2a), add all previous
+  // architecture versions and any dependant target features.
   llvm::AArch64::ArchKind AK = llvm::AArch64::getSubArchArchKind(Name);
-  // Add all previous architecture versions.
+  if (AK == llvm::AArch64::ArchKind::INVALID)
+    return;
   // In case of v9.x the v8.x counterparts are added too.
   if ("9" == getArchVersionString(AK))
     for (llvm::AArch64::ArchKind I = llvm::AArch64::convertV9toV8(AK);
          I != llvm::AArch64::ArchKind::INVALID; --I)
       Features[llvm::AArch64::getSubArch(I)] = Enabled;
 
-  for (llvm::AArch64::ArchKind I = --AK; I != llvm::AArch64::ArchKind::INVALID;
-       --I)
+  llvm::AArch64::ArchKind I = AK;
+  for (--I; I != llvm::AArch64::ArchKind::INVALID; --I)
     Features[llvm::AArch64::getSubArch(I)] = Enabled;
+
+  // Set any features implied by the architecture
+  uint64_t Extensions = llvm::AArch64::getDefaultExtensions("generic", AK);
+  std::vector<StringRef> CPUFeats;
+  if (llvm::AArch64::getExtensionFeatures(Extensions, CPUFeats)) {
+    for (auto F : CPUFeats) {
+      assert(F[0] == '+' && "Expected + in target feature!");
+      if (F == "+crypto")
+        continue;
+      Features[F.drop_front(1)] = true;
+    }
+  }
 }
 
 bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
                                              DiagnosticsEngine &Diags) {
   for (const auto &Feature : Features) {
     if (Feature == "-neon")
       HasNoNeon = true;
     if (Feature == "-sve")

clang/test/CodeGen/aarch64-targetattr-arch.c

+// RUN: %clang_cc1 -triple aarch64-eabi -target-feature +v8a -verify -DHAS8 -S %s -o -
+// RUN: %clang_cc1 -triple aarch64-eabi -target-feature +v8.1a -verify -DHAS81 -S %s -o -
+// RUN: %clang_cc1 -triple aarch64-eabi -target-feature +v9a -verify -DHAS9 -S %s -o -
+
+#ifdef HAS9
+// expected-no-diagnostics
+#endif
+
+#include <arm_acle.h>
+#include <arm_sve.h>
+
+__attribute__((target("arch=armv8.1-a")))
+int test_crc_attr()
+{
+  return __crc32cd(1, 1);
+}
+
+__attribute__((target("arch=armv9-a")))
+svint8_t test_svadd_attr(svbool_t pg, svint8_t op1, svint8_t op2)
+{
+  return svadd_s8_z(pg, op1, op2);
+}
+
+svint8_t test_errors(svbool_t pg, svint8_t op1, svint8_t op2)
+{
+#ifdef HAS8
+// expected-error@+2{{always_inline function '__crc32cd' requires target feature 'crc'}}
+#endif
+  __crc32cd(1, 1);
+#if defined(HAS8) || defined(HAS81)
+// expected-error@+2{{'svadd_s8_z' needs target feature sve}}
+#endif
+  return svadd_s8_z(pg, op1, op2);
+}

clang/test/CodeGen/aarch64-targetattr.c

 void plusnosimd() {}
 // CHECK-LABEL: @noneon() #17
 __attribute__((target("no-neon")))
 void noneon() {}
 // CHECK-LABEL: @nosimd() #17
 __attribute__((target("no-simd")))
 void nosimd() {}
 
-// CHECK: attributes #0 = { {{.*}} "target-features"="+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #1 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve,+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #2 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve,+sve2,+v8.1a,+v8.2a,+v8a" }
-// CHECK: attributes #3 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
+// CHECK: attributes #0 = { {{.*}} "target-features"="+crc,+fp-armv8,+lse,+neon,+ras,+rdm,+v8.1a,+v8.2a,+v8a" }
+// CHECK: attributes #1 = { {{.*}} "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+v8.1a,+v8.2a,+v8a" }
+// CHECK: attributes #2 = { {{.*}} "target-features"="+crc,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rdm,+sve,+sve2,+v8.1a,+v8.2a,+v8a" }
+// CHECK: attributes #3 = { {{.*}} "target-features"="+aes,+bf16,+crc,+dotprod,+fp-armv8,+fullfp16,+i8mm,+lse,+neon,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" }
 // CHECK: attributes #4 = { {{.*}} "target-cpu"="cortex-a710" "target-features"="+bf16,+crc,+dotprod,+flagm,+fp-armv8,+fp16fml,+i8mm,+lse,+mte,+neon,+pauth,+ras,+rcpc,+rdm,+sb,+sve,+sve2,+sve2-bitperm" }
 // CHECK: attributes #5 = { {{.*}} "tune-cpu"="cortex-a710" }
 // CHECK: attributes #6 = { {{.*}} "target-cpu"="generic" }
 // CHECK: attributes #7 = { {{.*}} "tune-cpu"="generic" }
 // CHECK: attributes #8 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+spe,+ssbs" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #9 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #10 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+bf16,+crc,+crypto,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+lse,+neon,+rand,+ras,+rcpc,+rdm,+spe,+ssbs,+sve,+sve2" }
 // CHECK: attributes #11 = { {{.*}} "target-cpu"="neoverse-v1" "target-features"="+bf16,+crc,+crypto,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+i8mm,+lse,+neon,+rand,+ras,+rcpc,+rdm,+spe,+ssbs,-sve" }
 // CHECK: attributes #12 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve" }
 // CHECK: attributes #13 = { {{.*}} "target-features"="+fp-armv8,+fullfp16,+neon,+sve,-sve2" }
 // CHECK: attributes #14 = { {{.*}} "target-features"="+fullfp16" }
-// CHECK: attributes #15 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
-// CHECK: attributes #16 = { {{.*}} "branch-target-enforcement"="true" {{.*}} "target-cpu"="neoverse-n1" "target-features"="+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
+// CHECK: attributes #15 = { {{.*}} "target-cpu"="neoverse-n1" "target-features"="+aes,+bf16,+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+i8mm,+lse,+neon,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
+// CHECK: attributes #16 = { {{.*}} "branch-target-enforcement"="true" {{.*}} "target-features"="+aes,+bf16,+crc,+crypto,+dotprod,+fp-armv8,+fullfp16,+i8mm,+lse,+neon,+ras,+rcpc,+rdm,+sha2,+sha3,+sm4,+spe,+ssbs,+sve,+sve2,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8.6a,+v8a" "tune-cpu"="cortex-a710" }
 // CHECK: attributes #17 = { {{.*}} "target-features"="-neon" }

llvm/include/llvm/TargetParser/AArch64TargetParser.def

AARCH64_ARCH("armv8.9-a", ARMV8_9A, "+v8.9a",
               AArch64::AEK_RDM | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
               AArch64::AEK_SM4 | AArch64::AEK_SHA3 | AArch64::AEK_BF16 |
               AArch64::AEK_SHA2 | AArch64::AEK_AES | AArch64::AEK_I8MM |
               AArch64::AEK_RASv2))
 AARCH64_ARCH("armv9-a",   ARMV9A, "+v9a",
              (AArch64::AEK_CRC | AArch64::AEK_FP |
               AArch64::AEK_SIMD | AArch64::AEK_RAS | AArch64::AEK_LSE |
               AArch64::AEK_RDM | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
-              AArch64::AEK_SVE2))
+              AArch64::AEK_SVE | AArch64::AEK_SVE2))
 AARCH64_ARCH("armv9.1-a", ARMV9_1A, "+v9.1a",
              (AArch64::AEK_CRC | AArch64::AEK_FP |
               AArch64::AEK_SIMD | AArch64::AEK_RAS | AArch64::AEK_LSE |
               AArch64::AEK_RDM | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
-              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE2))
+              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE |
+              AArch64::AEK_SVE2))
 AARCH64_ARCH("armv9.2-a", ARMV9_2A, "+v9.2a",
              (AArch64::AEK_CRC | AArch64::AEK_FP |
               AArch64::AEK_SIMD | AArch64::AEK_RAS | AArch64::AEK_LSE |
               AArch64::AEK_RDM | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
-              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE2))
+              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE |
+              AArch64::AEK_SVE2))
 AARCH64_ARCH("armv9.3-a", ARMV9_3A, "+v9.3a",
              (AArch64::AEK_CRC | AArch64::AEK_FP |
               AArch64::AEK_SIMD | AArch64::AEK_RAS | AArch64::AEK_LSE |
               AArch64::AEK_RDM | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
-              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE2))
+              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE |
+              AArch64::AEK_SVE2))
 AARCH64_ARCH("armv9.4-a", ARMV9_4A, "+v9.4a",
              (AArch64::AEK_CRC | AArch64::AEK_FP |
               AArch64::AEK_SIMD | AArch64::AEK_RAS | AArch64::AEK_LSE |
               AArch64::AEK_RDM | AArch64::AEK_RCPC | AArch64::AEK_DOTPROD |
-              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE2 |
-              AArch64::AEK_RASv2))
+              AArch64::AEK_BF16 | AArch64::AEK_I8MM | AArch64::AEK_SVE |
+              AArch64::AEK_SVE2 | AArch64::AEK_RASv2))
 // For v8-R, we do not enable crypto and align with GCC that enables a more
 // minimal set of optional architecture extensions.
 AARCH64_ARCH("armv8-r", ARMV8R, "+v8r",
              (AArch64::AEK_CRC     | AArch64::AEK_RDM  | AArch64::AEK_SSBS |
               AArch64::AEK_DOTPROD | AArch64::AEK_FP   | AArch64::AEK_SIMD |
               AArch64::AEK_FP16    | AArch64::AEK_FP16FML | AArch64::AEK_RAS |
               AArch64::AEK_RCPC    | AArch64::AEK_SB))
 #undef AARCH64_ARCH