[AArch64] Add all predecessor archs in target info

[danielkiss]

Summary

A given function is compatible with all previous arch versions.
To avoid compering values of the attribute this logic adds all predecessor
architecture values.

[dmgreen]

I think the idea makes sense. I was expecting some tests to change, where they were previously checking for "..,+v8.2a" and would now see v8.1a and v8a, etc.

clang/lib/Basic/Targets/AArch64.cpp

536

Why getCPUArchKind?

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

420 ↗

(On Diff #461862)

This looks like it is expecting to add new features just after the arch feature, which may not be the correct one. I'm not sure how much that matters but I guess the idea is that those features can later be disabled.

[DavidSpickett]

Can't approve because I don't know the motivation, but in general this is a good thing to add. I wish the target parser had a better way to model it so we didn't have to rely on the ordering of enum values but that's out of scope for now.

clang/lib/Basic/Targets/AArch64.cpp

538

I'd prefer that this sort of logic went in the target parser somewhere. Ok it's yet another one off function but I'd like to keep all these target query functions all in one place.

NormaliseV8V9? Horrible name but you get the idea.

639–640

Can you add a comment here explaining the intent of this?

I think it is something like, if the current architecture kind is not equal to what the feature would give you, make the current arch kind equal to the feature's architecture. So if ArchKind was v8.2 and I put in v8.4, we set ArchKind to 8.4. If it was 8.5 to start with, don't change it.

It's not so obvious on first read though.

llvm/include/llvm/Support/AArch64TargetParser.h

116

We need to at least add comments to the ArchKind definition in target parser to say that order matters now. They are in order but only because it made sense to list them that way.

Perhaps test the order in the target parser tests but that feels like repeating the same stuff and just another test we need to update. So compromise with a comment.

[DavidSpickett]

Cant accept the revision rather, I'll leave that to the others.

[danielkiss]

Refactored and added tests.

[dmgreen]

I think this looks OK. It is a good sign that I had something similar, even if the code came out quite differently.

There are a number of nitpicks below, the general idea LGTM. Thanks for the patch.

clang/lib/Basic/Targets/AArch64.cpp

535

Add a comment explaining that we add 8.x architectures based on 9.x, and another that we add all previous architectures.

537

Capitalize i. Here and below.

639–640

predeccor -> predecessor?

clang/test/CodeGen/aarch64-subarch-compatbility.c

5

shall able -> should be able?

llvm/unittests/Support/TargetParserTest.cpp

1590

Use EXPECT_EQ for these?

1596

Should <= be <?

1598–1599

It may be better to list the possibilities for current v9 architectures.

[DavidSpickett]

clang/lib/Basic/Targets/AArch64.cpp

543

It's not clear to me that you need 2 loops or the conditional here. What prevents you from just having:

llvm::AArch64::ArchKind AK = llvm::AArch64::getSubArchArchKind(Name);
for (llvm::AArch64::ArchKind i = llvm::AArch64::convertV9toV8(AK);
  i != llvm::AArch64::ArchKind::INVALID; --i)
    Features[llvm::AArch64::getSubArch(i)] = Enabled;

Given that converting anything that isn't v9 returns itself, then you walk backwards to all the previous architectures. Seems like you only need to do that once but tell me what I'm missing.

[danielkiss]

address comments.

clang/lib/Basic/Targets/AArch64.cpp

543

The iterator only works inside the architecture family 9/8.
so the first loop will set only the v8 counterparts while the second the v9.

[DavidSpickett]

My questions were answered, LGTM.

clang/lib/Basic/Targets/AArch64.cpp

543

Got it, I missed that your operator-- when applied to v9.0 will return invalid. I thought it would go to whatever last v8 there was.

[dmgreen]

Thanks. LGTM

clang/lib/Basic/Targets/AArch64.cpp

536

architechture -> architecture

clang/lib/Basic/Targets/AArch64.h

void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
   Optional<std::pair<unsigned, unsigned>>
   getVScaleRange(const LangOptions &LangOpts) const override;
 
   bool hasFeature(StringRef Feature) const override;
+  void setFeatureEnabled(llvm::StringMap<bool> &Features, StringRef Name,
+                         bool Enabled) const override;
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override;
 
   bool hasBFloat16Type() const override;
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override;
 
   bool isCLZForZeroUndef() const override;

clang/lib/Basic/Targets/AArch64.cpp

bool AArch64TargetInfo::hasFeature(StringRef Feature) const {
   return llvm::StringSwitch<bool>(Feature)
     .Cases("aarch64", "arm64", "arm", true)
     .Case("neon", FPU & NeonMode)
     .Cases("sve", "sve2", "sve2-bitperm", "sve2-aes", "sve2-sha3", "sve2-sm4", "f64mm", "f32mm", "i8mm", "bf16", FPU & SveMode)
     .Case("ls64", HasLS64)
     .Default(false);
 }
 
+void AArch64TargetInfo::setFeatureEnabled(llvm::StringMap<bool> &Features,
+                                          StringRef Name, bool Enabled) const {
+  Features[Name] = Enabled;
+  llvm::AArch64::ArchKind AK = llvm::AArch64::getSubArchArchKind(Name);
+  // Add all previous architecture versions.
+  // 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)
+    Features[llvm::AArch64::getSubArch(I)] = Enabled;
+}
+
 bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
                                              DiagnosticsEngine &Diags) {
   FPU = FPUMode;
   HasCRC = false;
   HasAES = false;
   HasSHA2 = false;
   HasSHA3 = false;
   HasSM4 = false;
for (const auto &Feature : Features) {
     if (Feature == "+sha3") {
       HasSHA2 = true;
       HasSHA3 = true;
     }
     if (Feature == "+sm4")
       HasSM4 = true;
     if (Feature == "+strict-align")
       HasUnaligned = false;
-    if (Feature == "+v8a")
+    // All predecessor archs are added but select the latest one for ArchKind.
+    if (Feature == "+v8a" && ArchKind < llvm::AArch64::ArchKind::ARMV8A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8A;
-    if (Feature == "+v8.1a")
+    if (Feature == "+v8.1a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_1A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_1A;
-    if (Feature == "+v8.2a")
+    if (Feature == "+v8.2a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_2A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_2A;
-    if (Feature == "+v8.3a")
+    if (Feature == "+v8.3a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_3A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_3A;
-    if (Feature == "+v8.4a")
+    if (Feature == "+v8.4a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_4A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_4A;
-    if (Feature == "+v8.5a")
+    if (Feature == "+v8.5a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_5A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_5A;
-    if (Feature == "+v8.6a")
+    if (Feature == "+v8.6a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_6A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_6A;
-    if (Feature == "+v8.7a")
+    if (Feature == "+v8.7a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_7A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_7A;
-    if (Feature == "+v8.8a")
+    if (Feature == "+v8.8a" && ArchKind < llvm::AArch64::ArchKind::ARMV8_8A)
       ArchKind = llvm::AArch64::ArchKind::ARMV8_8A;
-    if (Feature == "+v9a")
+    if (Feature == "+v9a" && ArchKind < llvm::AArch64::ArchKind::ARMV9A)
       ArchKind = llvm::AArch64::ArchKind::ARMV9A;
-    if (Feature == "+v9.1a")
+    if (Feature == "+v9.1a" && ArchKind < llvm::AArch64::ArchKind::ARMV9_1A)
       ArchKind = llvm::AArch64::ArchKind::ARMV9_1A;
-    if (Feature == "+v9.2a")
+    if (Feature == "+v9.2a" && ArchKind < llvm::AArch64::ArchKind::ARMV9_2A)
       ArchKind = llvm::AArch64::ArchKind::ARMV9_2A;
-    if (Feature == "+v9.3a")
+    if (Feature == "+v9.3a" && ArchKind < llvm::AArch64::ArchKind::ARMV9_3A)
       ArchKind = llvm::AArch64::ArchKind::ARMV9_3A;
     if (Feature == "+v8r")
       ArchKind = llvm::AArch64::ArchKind::ARMV8R;
     if (Feature == "+fullfp16")
       HasFullFP16 = true;
     if (Feature == "+dotprod")
       HasDotProd = true;
     if (Feature == "+fp16fml")

clang/test/CodeGen/aarch64-subarch-compatbility.c

+// REQUIRES: aarch64-registered-target
+// RUN: %clang -target aarch64-none-linux -march=armv9.3-a -o %t -c %s 2>&1 | FileCheck --allow-empty %s
+
+// Successor targets should be ableto call predecessor target functions.
+__attribute__((__always_inline__,target("v8a")))
+int armv80(int i) {
+    return i + 42;
+}
+
+__attribute__((__always_inline__,target("v8.1a")))
+int armv81(int i) {
+    return armv80(i);
+}
+
+__attribute__((__always_inline__,target("v8.2a")))
+int armv82(int i) {
+    return armv81(i);
+}
+
+__attribute__((__always_inline__,target("v8.3a")))
+int armv83(int i) {
+    return armv82(i);
+}
+
+__attribute__((__always_inline__,target("v8.4a")))
+int armv84(int i) {
+    return armv83(i);
+}
+
+__attribute__((__always_inline__,target("v8.5a")))
+int armv85(int i) {
+    return armv84(i);
+}
+
+__attribute__((__always_inline__,target("v8.6a")))
+int armv86(int i) {
+    return armv85(i);
+}
+
+__attribute__((__always_inline__,target("v8.7a")))
+int armv87(int i) {
+    return armv86(i);
+}
+
+__attribute__((__always_inline__,target("v8.8a")))
+int armv88(int i) {
+    return armv87(i);
+}
+
+__attribute__((__always_inline__,target("v9a")))
+int armv9(int i) {
+    return armv85(i);
+}
+
+__attribute__((__always_inline__,target("v9.1a")))
+int armv91(int i) {
+    return armv9(i);
+}
+
+__attribute__((__always_inline__,target("v9.2a")))
+int armv92(int i) {
+    return armv91(i);
+}
+
+__attribute__((__always_inline__,target("v9.3a")))
+int armv93(int i) {
+    return armv92(i);
+}
+
+// CHECK-NOT: always_inline function {{.*}} requires target feature {{.*}}, but would be inlined into function {{.*}} that is compiled without support for {{.*}}
+// CHECK-NOT: {{.*}} is not a recognized feature for this target

llvm/include/llvm/Support/AArch64TargetParser.h

 };
 
 const ArchKind ArchKinds[] = {
 #define AARCH64_ARCH(NAME, ID, CPU_ATTR, SUB_ARCH, ARCH_ATTR, ARCH_FPU, ARCH_BASE_EXT) \
     ArchKind::ID,
 #include "AArch64TargetParser.def"
 };
 
+inline ArchKind &operator--(ArchKind &Kind) {
+  if ((Kind == ArchKind::INVALID) || (Kind == ArchKind::ARMV8A) ||
+      (Kind == ArchKind::ARMV9A) || (Kind == ArchKind::ARMV8R))
+    Kind = ArchKind::INVALID;
+  else {
+    unsigned KindAsInteger = static_cast<unsigned>(Kind);
+    Kind = static_cast<ArchKind>(--KindAsInteger);
+  }
+  return Kind;
+}
+
 // FIXME: These should be moved to TargetTuple once it exists
 bool getExtensionFeatures(uint64_t Extensions,
                           std::vector<StringRef> &Features);
 bool getArchFeatures(ArchKind AK, std::vector<StringRef> &Features);
 
 StringRef getArchName(ArchKind AK);
 unsigned getArchAttr(ArchKind AK);
 StringRef getCPUAttr(ArchKind AK);
 StringRef getSubArch(ArchKind AK);
 StringRef getArchExtName(unsigned ArchExtKind);
 StringRef getArchExtFeature(StringRef ArchExt);
+ArchKind convertV9toV8(ArchKind AK);
 
 // Information by Name
 unsigned getDefaultFPU(StringRef CPU, ArchKind AK);
 uint64_t getDefaultExtensions(StringRef CPU, ArchKind AK);
 StringRef getDefaultCPU(StringRef Arch);
 ArchKind getCPUArchKind(StringRef CPU);
+ArchKind getSubArchArchKind(StringRef SubArch);
 
 // Parser
 ArchKind parseArch(StringRef Arch);
 ArchExtKind parseArchExt(StringRef ArchExt);
 ArchKind parseCPUArch(StringRef CPU);
 // Used by target parser tests
 void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values);
 
 bool isX18ReservedByDefault(const Triple &TT);
 
 } // namespace AArch64
 } // namespace llvm
 
 #endif

llvm/include/llvm/Support/AArch64TargetParser.def

 //
 //===----------------------------------------------------------------------===//
 
 // NOTE: NO INCLUDE GUARD DESIRED!
 
 #ifndef AARCH64_ARCH
 #define AARCH64_ARCH(NAME, ID, CPU_ATTR, SUB_ARCH, ARCH_ATTR, ARCH_FPU, ARCH_BASE_EXT)
 #endif
+// NOTE: The order and the grouping of the elements matter to make ArchKind iterable.
+// List is organised as armv8a -> armv8n-a, armv9a -> armv9m-a and armv8-r.
 AARCH64_ARCH("invalid", INVALID, "", "",
              ARMBuildAttrs::CPUArch::v8_A, FK_NONE, AArch64::AEK_NONE)
 AARCH64_ARCH("armv8-a", ARMV8A, "8-A", "v8a", ARMBuildAttrs::CPUArch::v8_A,
              FK_CRYPTO_NEON_FP_ARMV8,
              (AArch64::AEK_CRYPTO | AArch64::AEK_FP | AArch64::AEK_SIMD))
 AARCH64_ARCH("armv8.1-a", ARMV8_1A, "8.1-A", "v8.1a",
              ARMBuildAttrs::CPUArch::v8_A, FK_CRYPTO_NEON_FP_ARMV8,
              (AArch64::AEK_CRC | AArch64::AEK_CRYPTO | AArch64::AEK_FP |

llvm/lib/Support/AArch64TargetParser.cpp

AArch64::ArchKind AArch64::getCPUArchKind(StringRef CPU) {
 
   return StringSwitch<AArch64::ArchKind>(CPU)
 #define AARCH64_CPU_NAME(NAME, ID, DEFAULT_FPU, IS_DEFAULT, DEFAULT_EXT)       \
   .Case(NAME, ArchKind::ID)
 #include "../../include/llvm/Support/AArch64TargetParser.def"
   .Default(ArchKind::INVALID);
 }
 
+AArch64::ArchKind AArch64::getSubArchArchKind(StringRef SubArch) {
+  return StringSwitch<AArch64::ArchKind>(SubArch)
+#define AARCH64_ARCH(NAME, ID, CPU_ATTR, SUB_ARCH, ARCH_ATTR, ARCH_FPU,        \
+                     ARCH_BASE_EXT)                                            \
+  .Case(SUB_ARCH, ArchKind::ID)
+#include "../../include/llvm/Support/AArch64TargetParser.def"
+  .Default(ArchKind::INVALID);
+}
+
 bool AArch64::getExtensionFeatures(uint64_t Extensions,
                                    std::vector<StringRef> &Features) {
   if (Extensions == AArch64::AEK_INVALID)
     return false;
 
 #define AARCH64_ARCH_EXT_NAME(NAME, ID, FEATURE, NEGFEATURE)                   \
   if (Extensions & ID) {                                                       \
     const char *feature = FEATURE;                                             \
StringRef AArch64::getArchExtFeature(StringRef ArchExt) {
   }
 
   for (const auto &AE : AArch64ARCHExtNames)
     if (AE.Feature && ArchExt == AE.getName())
       return StringRef(AE.Feature);
   return StringRef();
 }
 
+AArch64::ArchKind AArch64::convertV9toV8(AArch64::ArchKind AK) {
+  if (AK == AArch64::ArchKind::INVALID)
+    return AK;
+  if (AK < AArch64::ArchKind::ARMV9A)
+    return AK;
+  if (AK >= AArch64::ArchKind::ARMV8R)
+    return AArch64::ArchKind::INVALID;
+  unsigned AK_v8 = static_cast<unsigned>(AArch64::ArchKind::ARMV8_5A);
+  AK_v8 += static_cast<unsigned>(AK) -
+           static_cast<unsigned>(AArch64::ArchKind::ARMV9A);
+  return static_cast<AArch64::ArchKind>(AK_v8);
+}
+
 StringRef AArch64::getDefaultCPU(StringRef Arch) {
   ArchKind AK = parseArch(Arch);
   if (AK == ArchKind::INVALID)
     return StringRef();
 
   // Look for multiple AKs to find the default for pair AK+Name.
   for (const auto &CPU : AArch64CPUNames)
     if (CPU.ArchID == AK && CPU.Default)

llvm/unittests/Support/TargetParserTest.cpp

TEST(TargetParserTest, AArch64ArchFeatures) {
   for (auto AK : AArch64::ArchKinds) {
     if (AK == AArch64::ArchKind::INVALID)
       EXPECT_FALSE(AArch64::getArchFeatures(AK, Features));
     else
       EXPECT_TRUE(AArch64::getArchFeatures(AK, Features));
   }
 }
 
+TEST(TargetParserTest, AArch64ArchV9toV8Conversion) {
+  for (auto AK : AArch64::ArchKinds) {
+    if (AK == AArch64::ArchKind::INVALID)
+      EXPECT_EQ(AK, AArch64::convertV9toV8(AK));
+    else if (AK < AArch64::ArchKind::ARMV9A)
+      EXPECT_EQ(AK, AArch64::convertV9toV8(AK));
+    else if (AK >= AArch64::ArchKind::ARMV8R)
+      EXPECT_EQ(AArch64::ArchKind::INVALID, AArch64::convertV9toV8(AK));
+    else
+      EXPECT_TRUE(AArch64::convertV9toV8(AK) < AArch64::ArchKind::ARMV9A);
+  }
+  EXPECT_EQ(AArch64::ArchKind::ARMV8_5A,
+              AArch64::convertV9toV8(AArch64::ArchKind::ARMV9A));
+  EXPECT_EQ(AArch64::ArchKind::ARMV8_6A,
+              AArch64::convertV9toV8(AArch64::ArchKind::ARMV9_1A));
+  EXPECT_EQ(AArch64::ArchKind::ARMV8_7A,
+              AArch64::convertV9toV8(AArch64::ArchKind::ARMV9_2A));
+  EXPECT_EQ(AArch64::ArchKind::ARMV8_8A,
+              AArch64::convertV9toV8(AArch64::ArchKind::ARMV9_3A));
+}
+
 TEST(TargetParserTest, AArch64ArchExtFeature) {
   const char *ArchExt[][4] = {
       {"crc", "nocrc", "+crc", "-crc"},
       {"crypto", "nocrypto", "+crypto", "-crypto"},
       {"flagm", "noflagm", "+flagm", "-flagm"},
       {"fp", "nofp", "+fp-armv8", "-fp-armv8"},
       {"simd", "nosimd", "+neon", "-neon"},
       {"fp16", "nofp16", "+fullfp16", "-fullfp16"},