[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

531

Why getCPUArchKind?

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

420

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

533

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.

634

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

530

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

532

Capitalize i. Here and below.

634

predeccor -> predecessor?

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

4 ↗

(On Diff #462506)

shall able -> should be able?

llvm/unittests/Support/TargetParserTest.cpp

1579 ↗

(On Diff #462506)

Use EXPECT_EQ for these?

1585 ↗

(On Diff #462506)

Should <= be <?

1587–1588 ↗

(On Diff #462506)

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

[DavidSpickett]

clang/lib/Basic/Targets/AArch64.cpp

538

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

538

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

538

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

531

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::getCPUArchKind(Name);
+  if ("9" == getArchVersionString(AK)) {
+    unsigned AK_v85 = static_cast<unsigned>(llvm::AArch64::ArchKind::ARMV8_5A);
+    AK_v85 += static_cast<unsigned>(AK) -
+              static_cast<unsigned>(llvm::AArch64::ArchKind::ARMV9A);
+    for (llvm::AArch64::ArchKind i =
+             static_cast<llvm::AArch64::ArchKind>(AK_v85);
+         i != llvm::AArch64::ArchKind::INVALID; --i)
+      Features[llvm::AArch64::getSubArch(i)] = Enabled;
+  }
+  for (llvm::AArch64::ArchKind i = --llvm::AArch64::getCPUArchKind(Name);
+       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")
+    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/lib/Driver/ToolChains/Arch/AArch64.cpp

void aarch64::getAArch64TargetFeatures(const Driver &D,
   bool HasNoCrypto = false;
   int FullFP16Pos = -1;
   int NoFullFP16Pos = -1;
   int FP16FMLPos = -1;
   int NoFP16FMLPos = -1;
   int ArchFeatPos = -1;
 
   for (auto I = Features.begin(), E = Features.end(); I != E; I++) {
-    if (*I == "+v8a")   V8Version = 0;
-    else if (*I == "+v8.1a") V8Version = 1;
-    else if (*I == "+v8.2a") V8Version = 2;
-    else if (*I == "+v8.3a") V8Version = 3;
-    else if (*I == "+v8.4a") V8Version = 4;
-    else if (*I == "+v8.5a") V8Version = 5;
-    else if (*I == "+v8.6a") V8Version = 6;
-    else if (*I == "+v8.7a") V8Version = 7;
-    else if (*I == "+v8.8a") V8Version = 8;
-    else if (*I == "+v8.9a") V8Version = 9;
-    else if (*I == "+v9a")   V9Version = 0;
-    else if (*I == "+v9.1a") V9Version = 1;
-    else if (*I == "+v9.2a") V9Version = 2;
-    else if (*I == "+v9.3a") V9Version = 3;
-    else if (*I == "+v9.4a") V9Version = 4;
+    if (*I == "+v8a" && V8Version < 0)
+      V8Version = 0;
+    else if (*I == "+v8.1a" && V8Version < 1)
+      V8Version = 1;
+    else if (*I == "+v8.2a" && V8Version < 2)
+      V8Version = 2;
+    else if (*I == "+v8.3a" && V8Version < 3)
+      V8Version = 3;
+    else if (*I == "+v8.4a" && V8Version < 4)
+      V8Version = 4;
+    else if (*I == "+v8.5a" && V8Version < 5)
+      V8Version = 5;
+    else if (*I == "+v8.6a" && V8Version < 6)
+      V8Version = 6;
+    else if (*I == "+v8.7a" && V8Version < 7)
+      V8Version = 7;
+    else if (*I == "+v8.8a" && V8Version < 8)
+      V8Version = 8;
+    else if (*I == "+v8.9a" && V8Version < 9)
+      V8Version = 9;
+    else if (*I == "+v9a" && V9Version < 0)
+      V9Version = 0;
+    else if (*I == "+v9.1a" && V9Version < 1)
+      V9Version = 1;
+    else if (*I == "+v9.2a" && V9Version < 2)
+      V9Version = 2;
+    else if (*I == "+v9.3a" && V9Version < 3)
+      V9Version = 3;
+    else if (*I == "+v9.4a" && V9Version < 4)
+      V9Version = 4;
     else if (*I == "+sm4")  HasSM4 = true;
     else if (*I == "+sha3") HasSHA3 = true;
     else if (*I == "+sha2") HasSHA2 = true;
     else if (*I == "+aes")  HasAES = true;
     else if (*I == "-sm4")  HasNoSM4 = true;
     else if (*I == "-sha3") HasNoSHA3 = true;
     else if (*I == "-sha2") HasNoSHA2 = true;
     else if (*I == "-aes")  HasNoAES = true;
     else if (*I == "+fp16fml")  FP16FMLPos = I - Features.begin();
     else if (*I == "-fp16fml")  NoFP16FMLPos = I - Features.begin();
     else if (*I == "-fullfp16") NoFullFP16Pos = I - Features.begin();
     else if (*I == "+fullfp16") FullFP16Pos = I - Features.begin();
     // Whichever option comes after (right-most option) will win
     else if (*I == "+crypto") {
       HasCrypto = true;
       HasNoCrypto = false;
     } else if (*I == "-crypto") {
       HasCrypto = false;
       HasNoCrypto = true;
     }
     // Register the iterator position if this is an architecture feature
     if (ArchFeatPos == -1 && (V8Version != -1 || V9Version != -1))
       ArchFeatPos = I - Features.begin();
   }
 
   // Handle (arch-dependent) fp16fml/fullfp16 relationship.
   // FIXME: this fp16fml option handling will be reimplemented after the
   // TargetParser rewrite.
   if (V8Version >= 4) {
     // "-fullfp16" "+fullfp16" && "+fp16fml" "+fullfp16" && no "+fullfp16" "-fp16fml" = "+fp16fml"

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)
+    return ArchKind::INVALID;
+  if (Kind == ArchKind::ARMV8A)
+    return ArchKind::INVALID;
+  if (Kind == ArchKind::ARMV9A)
+    return ArchKind::INVALID;
+  if (Kind == ArchKind::ARMV8R)
+    return ArchKind::INVALID;
+  unsigned KindAsInteger = static_cast<unsigned>(Kind);
+  return static_cast<ArchKind>(--KindAsInteger);
+}
+
 // 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);