[AArch64TargetParser] getArchFeatures -> getArchFeature

[pratlucas]

llvm/unittests/Support/TargetParserTest.cpp

1686

Can you keep a unit test covering the new version of the function?

[tmatheson]

llvm/unittests/Support/TargetParserTest.cpp

1686

The old version had special treatment for INVALID, but the new version is just returning the feature as written in AArch64TargetParser.def. A unit test would consist of a list of getArchFeature() calls with the same strings as in the .def. So I could certainly add it but I'm not sure that it adds any value as a unit test?

[tmatheson]

Reinstate unit test

[pratlucas]

LGTM

clang/lib/Basic/Targets/AArch64.cpp

if (Feature.startswith("arch=")) {
         Ret.Duplicate = "arch=";
       FoundArch = true;
       std::pair<StringRef, StringRef> Split =
           Feature.split("=").second.trim().split("+");
       llvm::AArch64::ArchKind ArchKind = llvm::AArch64::parseArch(Split.first);
 
       // Parse the architecture version, adding the required features to
       // Ret.Features.
-      std::vector<StringRef> FeatureStrs;
-      if (ArchKind == llvm::AArch64::ArchKind::INVALID ||
-          !llvm::AArch64::getArchFeatures(ArchKind, FeatureStrs))
+      if (ArchKind == llvm::AArch64::ArchKind::INVALID)
         continue;
-      for (auto R : FeatureStrs)
-        Ret.Features.push_back(R.str());
+      Ret.Features.push_back(llvm::AArch64::getArchFeature(ArchKind).str());
       // Add any extra features, after the +
       SplitAndAddFeatures(Split.second, Ret.Features);
     } else if (Feature.startswith("cpu=")) {
       if (!Ret.CPU.empty())
         Ret.Duplicate = "cpu=";
       else {
         // Split the cpu string into "cpu=", "cortex-a710" and any remaining
         // "+feat" features.

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

static bool DecodeAArch64Mcpu(const Driver &D, StringRef Mcpu, StringRef &CPU,
 
   if (CPU == "native")
     CPU = llvm::sys::getHostCPUName();
 
   if (CPU == "generic") {
     Features.push_back("+neon");
   } else {
     ArchKind = llvm::AArch64::parseCPUArch(CPU);
-    if (!llvm::AArch64::getArchFeatures(ArchKind, Features))
+    if (ArchKind == llvm::AArch64::ArchKind::INVALID)
       return false;
+    Features.push_back(llvm::AArch64::getArchFeature(ArchKind));
 
     uint64_t Extension = llvm::AArch64::getDefaultExtensions(CPU, ArchKind);
     if (!llvm::AArch64::getExtensionFeatures(Extension, Features))
       return false;
    }
 
    if (Split.second.size() &&
        !DecodeAArch64Features(D, Split.second, Features, ArchKind))
      return false;
 
    return true;
 }
 
 static bool
 getAArch64ArchFeaturesFromMarch(const Driver &D, StringRef March,
                                 const ArgList &Args,
                                 std::vector<StringRef> &Features) {
   std::string MarchLowerCase = March.lower();
   std::pair<StringRef, StringRef> Split = StringRef(MarchLowerCase).split("+");
 
   llvm::AArch64::ArchKind ArchKind = llvm::AArch64::parseArch(Split.first);
   if (Split.first == "native")
     ArchKind = llvm::AArch64::getCPUArchKind(llvm::sys::getHostCPUName().str());
-  if (ArchKind == llvm::AArch64::ArchKind::INVALID ||
-      !llvm::AArch64::getArchFeatures(ArchKind, Features))
+  if (ArchKind == llvm::AArch64::ArchKind::INVALID)
     return false;
+  Features.push_back(llvm::AArch64::getArchFeature(ArchKind));
 
   // Enable SVE2 by default on Armv9-A.
   // It can still be disabled if +nosve2 is present.
   // We must do this early so that DecodeAArch64Features has the correct state
   if ((ArchKind == llvm::AArch64::ArchKind::ARMV9A ||
        ArchKind == llvm::AArch64::ArchKind::ARMV9_1A ||
        ArchKind == llvm::AArch64::ArchKind::ARMV9_2A)) {
     Features.push_back("+sve");

llvm/include/llvm/Support/AArch64TargetParser.h

else {
     unsigned KindAsInteger = static_cast<unsigned>(Kind);
     Kind = static_cast<ArchKind>(--KindAsInteger);
   }
   return Kind;
 }
 
 bool getExtensionFeatures(uint64_t Extensions,
                           std::vector<StringRef> &Features);
-bool getArchFeatures(ArchKind AK, std::vector<StringRef> &Features);
+StringRef getArchFeature(ArchKind AK);
 
 StringRef getArchName(ArchKind AK);
 StringRef getSubArch(ArchKind AK);
 StringRef getArchExtName(unsigned ArchExtKind);
 StringRef getArchExtFeature(StringRef ArchExt);
 ArchKind convertV9toV8(ArchKind AK);
 StringRef resolveCPUAlias(StringRef CPU);
 

llvm/lib/Support/AArch64TargetParser.cpp

 StringRef AArch64::resolveCPUAlias(StringRef CPU) {
   return StringSwitch<StringRef>(CPU)
 #define AARCH64_CPU_ALIAS(ALIAS,NAME)                                          \
   .Case(ALIAS, NAME)
 #include "../../include/llvm/Support/AArch64TargetParser.def"
   .Default(CPU);
 }
 
-bool AArch64::getArchFeatures(AArch64::ArchKind AK,
-                              std::vector<StringRef> &Features) {
-  if (AK == ArchKind::INVALID)
-    return false;
-  Features.push_back(AArch64ARCHNames[static_cast<unsigned>(AK)].ArchFeature);
-  return true;
+StringRef AArch64::getArchFeature(AArch64::ArchKind AK) {
+  return AArch64ARCHNames[static_cast<unsigned>(AK)].ArchFeature;
 }
 
 StringRef AArch64::getArchName(AArch64::ArchKind AK) {
   return AArch64ARCHNames[static_cast<unsigned>(AK)].Name;
 }
 
 StringRef AArch64::getSubArch(AArch64::ArchKind AK) {
   return AArch64ARCHNames[static_cast<unsigned>(AK)].getSubArch();

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

bool AArch64AsmParser::parseDirectiveArch(SMLoc L) {
   if (ID == AArch64::ArchKind::INVALID)
     return Error(ArchLoc, "unknown arch name");
 
   if (parseToken(AsmToken::EndOfStatement))
     return true;
 
   // Get the architecture and extension features.
   std::vector<StringRef> AArch64Features;
-  AArch64::getArchFeatures(ID, AArch64Features);
+  AArch64Features.push_back(AArch64::getArchFeature(ID));
   AArch64::getExtensionFeatures(AArch64::getDefaultExtensions("generic", ID),
                                 AArch64Features);
 
   MCSubtargetInfo &STI = copySTI();
   std::vector<std::string> ArchFeatures(AArch64Features.begin(), AArch64Features.end());
   STI.setDefaultFeatures("generic", /*TuneCPU*/ "generic",
                          join(ArchFeatures.begin(), ArchFeatures.end(), ","));
 

llvm/unittests/Support/TargetParserTest.cpp

TEST(TargetParserTest, AArch64ExtensionFeatures) {
   // Assuming we listed every extension above, this should produce the same
   // result. (note that AEK_NONE doesn't have a name so it won't be in the
   // result despite its bit being set)
   std::vector<StringRef> AllFeatures;
   EXPECT_TRUE(AArch64::getExtensionFeatures(-1, AllFeatures));
   EXPECT_THAT(Features, ::testing::ContainerEq(AllFeatures));
 }
 
-TEST(TargetParserTest, AArch64ArchFeatures) {
-  std::vector<StringRef> Features;
-
-  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));