[llvm] [clang] [RISCV] Deduplicate RISCVISAInfo::toFeatures/toFeatureVector. NFC (PR #76942)

Luke Lau via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 8 22:23:07 PST 2024


https://github.com/lukel97 updated https://github.com/llvm/llvm-project/pull/76942

>From caa25a73dd69268490c89d5e9e91b8d545bce760 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Thu, 4 Jan 2024 14:02:39 +0900
Subject: [PATCH 1/6] [RISCV] Deduplicate
 RISCVISAInfo::toFeatures/toFeatureVector. NFC

toFeatures and toFeatureVector both output a list of target feature flags, just
with a slightly different interface. toFeatures keeps any unsupported
extensions, and also provides a way to append negative extensions
(AddAllExtensions=true).

This patch combines them into one function, so that a later patch will be be
able to get a std::vector of features that includes all the negative
extensions, which was previously only possible through the StrAlloc interface.
---
 clang/lib/Basic/Targets/RISCV.cpp           |  6 ++--
 clang/lib/Driver/ToolChains/Arch/RISCV.cpp  |  6 ++--
 llvm/include/llvm/Support/RISCVISAInfo.h    |  6 ++--
 llvm/lib/Object/ELFObjectFile.cpp           |  2 +-
 llvm/lib/Support/RISCVISAInfo.cpp           | 38 +++++++--------------
 llvm/unittests/Support/RISCVISAInfoTest.cpp | 30 +++++++++++++---
 6 files changed, 46 insertions(+), 42 deletions(-)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index 6bc57a83a2d5ae..64f5f9e9215dcb 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -245,7 +245,7 @@ collectNonISAExtFeature(ArrayRef<std::string> FeaturesNeedOverride, int XLen) {
     return std::vector<std::string>();
   }
 
-  std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatures();
 
   std::vector<std::string> NonISAExtFeatureVec;
 
@@ -303,7 +303,7 @@ bool RISCVTargetInfo::initFeatureMap(
   }
 
   // RISCVISAInfo makes implications for ISA features
-  std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  std::vector<std::string> ImpliedFeatures = (*ParseResult)->toFeatures();
 
   // parseFeatures normalizes the feature set by dropping any explicit
   // negatives, and non-extension features.  We need to preserve the later
@@ -420,7 +420,7 @@ static void handleFullArchString(StringRef FullArchStr,
     // Forward the invalid FullArchStr.
     Features.push_back("+" + FullArchStr.str());
   } else {
-    std::vector<std::string> FeatStrings = (*RII)->toFeatureVector();
+    std::vector<std::string> FeatStrings = (*RII)->toFeatures();
     Features.insert(Features.end(), FeatStrings.begin(), FeatStrings.end());
   }
 }
diff --git a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
index 0717e3b813e1e2..b97224426b916a 100644
--- a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
@@ -42,9 +42,9 @@ static bool getArchFeatures(const Driver &D, StringRef Arch,
     return false;
   }
 
-  (*ISAInfo)->toFeatures(
-      Features, [&Args](const Twine &Str) { return Args.MakeArgString(Str); },
-      /*AddAllExtensions=*/true);
+  for (std::string &Str : (*ISAInfo)->toFeatures(/*AddAllExtension=*/true,
+                                                 /*IgnoreUnknown=*/false))
+    Features.push_back(Args.MakeArgString(Str));
 
   if (EnableExperimentalExtensions)
     Features.push_back(Args.MakeArgString("+experimental"));
diff --git a/llvm/include/llvm/Support/RISCVISAInfo.h b/llvm/include/llvm/Support/RISCVISAInfo.h
index 09c4edd6df60e9..c539448683d368 100644
--- a/llvm/include/llvm/Support/RISCVISAInfo.h
+++ b/llvm/include/llvm/Support/RISCVISAInfo.h
@@ -68,9 +68,8 @@ class RISCVISAInfo {
   parseFeatures(unsigned XLen, const std::vector<std::string> &Features);
 
   /// Convert RISC-V ISA info to a feature vector.
-  void toFeatures(std::vector<StringRef> &Features,
-                  llvm::function_ref<StringRef(const Twine &)> StrAlloc,
-                  bool AddAllExtensions) const;
+  std::vector<std::string> toFeatures(bool AddAllExtensions = false,
+                                      bool IgnoreUnknown = true) const;
 
   const OrderedExtensionMap &getExtensions() const { return Exts; };
 
@@ -83,7 +82,6 @@ class RISCVISAInfo {
 
   bool hasExtension(StringRef Ext) const;
   std::string toString() const;
-  std::vector<std::string> toFeatureVector() const;
   StringRef computeDefaultABI() const;
 
   static bool isSupportedExtensionFeature(StringRef Ext);
diff --git a/llvm/lib/Object/ELFObjectFile.cpp b/llvm/lib/Object/ELFObjectFile.cpp
index 95c4f9f8545db2..ae21b81c10c82a 100644
--- a/llvm/lib/Object/ELFObjectFile.cpp
+++ b/llvm/lib/Object/ELFObjectFile.cpp
@@ -315,7 +315,7 @@ Expected<SubtargetFeatures> ELFObjectFileBase::getRISCVFeatures() const {
     else
       llvm_unreachable("XLEN should be 32 or 64.");
 
-    Features.addFeaturesVector(ISAInfo->toFeatureVector());
+    Features.addFeaturesVector(ISAInfo->toFeatures());
   }
 
   return Features;
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index a9b7e209915a13..6d267fae5a5dc6 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -466,35 +466,37 @@ bool RISCVISAInfo::compareExtension(const std::string &LHS,
   return LHS < RHS;
 }
 
-void RISCVISAInfo::toFeatures(
-    std::vector<StringRef> &Features,
-    llvm::function_ref<StringRef(const Twine &)> StrAlloc,
-    bool AddAllExtensions) const {
+std::vector<std::string> RISCVISAInfo::toFeatures(bool AddAllExtensions,
+                                                  bool IgnoreUnknown) const {
+  std::vector<std::string> Features;
   for (auto const &Ext : Exts) {
-    StringRef ExtName = Ext.first;
+    std::string ExtName = Ext.first;
 
-    if (ExtName == "i")
+    if (ExtName == "i") // i is not recognized in clang -cc1
+      continue;
+    if (IgnoreUnknown && !isSupportedExtension(ExtName))
       continue;
 
     if (isExperimentalExtension(ExtName)) {
-      Features.push_back(StrAlloc("+experimental-" + ExtName));
+      Features.push_back("+experimental-" + ExtName);
     } else {
-      Features.push_back(StrAlloc("+" + ExtName));
+      Features.push_back("+" + ExtName);
     }
   }
   if (AddAllExtensions) {
     for (const RISCVSupportedExtension &Ext : SupportedExtensions) {
       if (Exts.count(Ext.Name))
         continue;
-      Features.push_back(StrAlloc(Twine("-") + Ext.Name));
+      Features.push_back("-" + std::string(Ext.Name));
     }
 
     for (const RISCVSupportedExtension &Ext : SupportedExperimentalExtensions) {
       if (Exts.count(Ext.Name))
         continue;
-      Features.push_back(StrAlloc(Twine("-experimental-") + Ext.Name));
+      Features.push_back("-experimental-" + std::string(Ext.Name));
     }
   }
+  return Features;
 }
 
 // Extensions may have a version number, and may be separated by
@@ -1269,22 +1271,6 @@ std::string RISCVISAInfo::toString() const {
   return Arch.str();
 }
 
-std::vector<std::string> RISCVISAInfo::toFeatureVector() const {
-  std::vector<std::string> FeatureVector;
-  for (auto const &Ext : Exts) {
-    std::string ExtName = Ext.first;
-    if (ExtName == "i") // i is not recognized in clang -cc1
-      continue;
-    if (!isSupportedExtension(ExtName))
-      continue;
-    std::string Feature = isExperimentalExtension(ExtName)
-                              ? "+experimental-" + ExtName
-                              : "+" + ExtName;
-    FeatureVector.push_back(Feature);
-  }
-  return FeatureVector;
-}
-
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 RISCVISAInfo::postProcessAndChecking(std::unique_ptr<RISCVISAInfo> &&ISAInfo) {
   ISAInfo->updateImplication();
diff --git a/llvm/unittests/Support/RISCVISAInfoTest.cpp b/llvm/unittests/Support/RISCVISAInfoTest.cpp
index 7463824b5b5248..42759f30fd1bc3 100644
--- a/llvm/unittests/Support/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/Support/RISCVISAInfoTest.cpp
@@ -477,25 +477,45 @@ TEST(ParseArchString, RejectsConflictingExtensions) {
   }
 }
 
-TEST(ToFeatureVector, IIsDroppedAndExperimentalExtensionsArePrefixed) {
+TEST(ToFeatures, IIsDroppedAndExperimentalExtensionsArePrefixed) {
   auto MaybeISAInfo1 =
       RISCVISAInfo::parseArchString("rv64im_zicond", true, false);
   ASSERT_THAT_EXPECTED(MaybeISAInfo1, Succeeded());
-  EXPECT_THAT((*MaybeISAInfo1)->toFeatureVector(),
+  EXPECT_THAT((*MaybeISAInfo1)->toFeatures(),
               ElementsAre("+m", "+experimental-zicond"));
 
   auto MaybeISAInfo2 = RISCVISAInfo::parseArchString(
       "rv32e_zicond_xventanacondops", true, false);
   ASSERT_THAT_EXPECTED(MaybeISAInfo2, Succeeded());
-  EXPECT_THAT((*MaybeISAInfo2)->toFeatureVector(),
+  EXPECT_THAT((*MaybeISAInfo2)->toFeatures(),
               ElementsAre("+e", "+experimental-zicond", "+xventanacondops"));
 }
 
-TEST(ToFeatureVector, UnsupportedExtensionsAreDropped) {
+TEST(ToFeatures, UnsupportedExtensionsAreDropped) {
   auto MaybeISAInfo =
       RISCVISAInfo::parseNormalizedArchString("rv64i2p0_m2p0_xmadeup1p0");
   ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
-  EXPECT_THAT((*MaybeISAInfo)->toFeatureVector(), ElementsAre("+m"));
+  EXPECT_THAT((*MaybeISAInfo)->toFeatures(), ElementsAre("+m"));
+}
+
+TEST(ToFeatures, UnsupportedExtensionsAreKeptIfIgnoreUnknownIsFalse) {
+  auto MaybeISAInfo =
+      RISCVISAInfo::parseNormalizedArchString("rv64i2p0_m2p0_xmadeup1p0");
+  ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+  EXPECT_THAT((*MaybeISAInfo)->toFeatures(false, false),
+              ElementsAre("+m", "+xmadeup"));
+}
+
+TEST(ToFeatures, AddAllExtensionsAddsNegativeExtensions) {
+  auto MaybeISAInfo = RISCVISAInfo::parseNormalizedArchString("rv64i2p0_m2p0");
+  ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+
+  auto Features = (*MaybeISAInfo)->toFeatures(true);
+  EXPECT_GT(Features.size(), 1UL);
+  EXPECT_EQ(Features.front(), "+m");
+  // Every feature after should be a negative feature
+  for (auto &NegativeExt : llvm::drop_begin(Features))
+    EXPECT_TRUE(NegativeExt.substr(0, 1) == "-");
 }
 
 TEST(OrderedExtensionMap, ExtensionsAreCorrectlyOrdered) {

>From b4f7ab5bee8a4473b8fbf6373d08b74d6fa5e998 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Fri, 5 Jan 2024 09:46:12 +0700
Subject: [PATCH 2/6] Defer copying to std::string, use const reference

---
 clang/lib/Driver/ToolChains/Arch/RISCV.cpp | 4 ++--
 llvm/lib/Support/RISCVISAInfo.cpp          | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
index b97224426b916a..16a8b3cc42bab4 100644
--- a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
@@ -42,8 +42,8 @@ static bool getArchFeatures(const Driver &D, StringRef Arch,
     return false;
   }
 
-  for (std::string &Str : (*ISAInfo)->toFeatures(/*AddAllExtension=*/true,
-                                                 /*IgnoreUnknown=*/false))
+  for (const std::string &Str : (*ISAInfo)->toFeatures(/*AddAllExtension=*/true,
+                                                       /*IgnoreUnknown=*/false))
     Features.push_back(Args.MakeArgString(Str));
 
   if (EnableExperimentalExtensions)
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 6d267fae5a5dc6..7b4fe4e69779ff 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -470,7 +470,7 @@ std::vector<std::string> RISCVISAInfo::toFeatures(bool AddAllExtensions,
                                                   bool IgnoreUnknown) const {
   std::vector<std::string> Features;
   for (auto const &Ext : Exts) {
-    std::string ExtName = Ext.first;
+    StringRef ExtName = Ext.first;
 
     if (ExtName == "i") // i is not recognized in clang -cc1
       continue;
@@ -478,9 +478,9 @@ std::vector<std::string> RISCVISAInfo::toFeatures(bool AddAllExtensions,
       continue;
 
     if (isExperimentalExtension(ExtName)) {
-      Features.push_back("+experimental-" + ExtName);
+      Features.push_back("+experimental-" + std::string(ExtName));
     } else {
-      Features.push_back("+" + ExtName);
+      Features.push_back("+" + std::string(ExtName));
     }
   }
   if (AddAllExtensions) {

>From c57a3679e2ab140e08779e5f68f75e7630fdda6e Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Fri, 5 Jan 2024 09:58:18 +0700
Subject: [PATCH 3/6] Use destructuring, use llvm::Twine to avoid double string
 allocation

---
 llvm/lib/Support/RISCVISAInfo.cpp | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 7b4fe4e69779ff..7681f4ecced28a 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -469,31 +469,29 @@ bool RISCVISAInfo::compareExtension(const std::string &LHS,
 std::vector<std::string> RISCVISAInfo::toFeatures(bool AddAllExtensions,
                                                   bool IgnoreUnknown) const {
   std::vector<std::string> Features;
-  for (auto const &Ext : Exts) {
-    StringRef ExtName = Ext.first;
-
+  for (const auto &[ExtName, _] : Exts) {
     if (ExtName == "i") // i is not recognized in clang -cc1
       continue;
     if (IgnoreUnknown && !isSupportedExtension(ExtName))
       continue;
 
     if (isExperimentalExtension(ExtName)) {
-      Features.push_back("+experimental-" + std::string(ExtName));
+      Features.push_back((llvm::Twine("+experimental-") + ExtName).str());
     } else {
-      Features.push_back("+" + std::string(ExtName));
+      Features.push_back((llvm::Twine("+") + ExtName).str());
     }
   }
   if (AddAllExtensions) {
     for (const RISCVSupportedExtension &Ext : SupportedExtensions) {
       if (Exts.count(Ext.Name))
         continue;
-      Features.push_back("-" + std::string(Ext.Name));
+      Features.push_back((llvm::Twine("-") + Ext.Name).str());
     }
 
     for (const RISCVSupportedExtension &Ext : SupportedExperimentalExtensions) {
       if (Exts.count(Ext.Name))
         continue;
-      Features.push_back("-experimental-" + std::string(Ext.Name));
+      Features.push_back((llvm::Twine("-experimental-") + Ext.Name).str());
     }
   }
   return Features;

>From f70e1462fd28969ff5953d73e43a7caaf76494d3 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Fri, 5 Jan 2024 10:05:13 +0700
Subject: [PATCH 4/6] Use std::vector insert instead of push_back loop

---
 clang/lib/Driver/ToolChains/Arch/RISCV.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
index 16a8b3cc42bab4..ef7ca7deb79a38 100644
--- a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
@@ -42,9 +42,10 @@ static bool getArchFeatures(const Driver &D, StringRef Arch,
     return false;
   }
 
-  for (const std::string &Str : (*ISAInfo)->toFeatures(/*AddAllExtension=*/true,
-                                                       /*IgnoreUnknown=*/false))
-    Features.push_back(Args.MakeArgString(Str));
+  const auto ISAInfoFeatures = (*ISAInfo)->toFeatures(/*AddAllExtension=*/true,
+                                                      /*IgnoreUnknown=*/false);
+  Features.insert(Features.end(), ISAInfoFeatures.begin(),
+                  ISAInfoFeatures.end());
 
   if (EnableExperimentalExtensions)
     Features.push_back(Args.MakeArgString("+experimental"));

>From ee596d4b742e2bbd8ccd4918b90147ee97e3a498 Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Fri, 5 Jan 2024 13:10:07 +0700
Subject: [PATCH 5/6] Use push_back loop again (forgot we need to use
 makeargstring)

---
 clang/lib/Driver/ToolChains/Arch/RISCV.cpp | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
index ef7ca7deb79a38..16a8b3cc42bab4 100644
--- a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
@@ -42,10 +42,9 @@ static bool getArchFeatures(const Driver &D, StringRef Arch,
     return false;
   }
 
-  const auto ISAInfoFeatures = (*ISAInfo)->toFeatures(/*AddAllExtension=*/true,
-                                                      /*IgnoreUnknown=*/false);
-  Features.insert(Features.end(), ISAInfoFeatures.begin(),
-                  ISAInfoFeatures.end());
+  for (const std::string &Str : (*ISAInfo)->toFeatures(/*AddAllExtension=*/true,
+                                                       /*IgnoreUnknown=*/false))
+    Features.push_back(Args.MakeArgString(Str));
 
   if (EnableExperimentalExtensions)
     Features.push_back(Args.MakeArgString("+experimental"));

>From fa12f6be9856261561c60082ed1ff61469a4644b Mon Sep 17 00:00:00 2001
From: Luke Lau <luke at igalia.com>
Date: Tue, 9 Jan 2024 13:22:30 +0700
Subject: [PATCH 6/6] Add a link to the specification to explain that i is not
 an extension

---
 llvm/lib/Support/RISCVISAInfo.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 7681f4ecced28a..70f531e40b90e6 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -470,7 +470,10 @@ std::vector<std::string> RISCVISAInfo::toFeatures(bool AddAllExtensions,
                                                   bool IgnoreUnknown) const {
   std::vector<std::string> Features;
   for (const auto &[ExtName, _] : Exts) {
-    if (ExtName == "i") // i is not recognized in clang -cc1
+    // i is a base instruction set, not an extension (see
+    // https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc#base-integer-isa)
+    // and is not recognized in clang -cc1
+    if (ExtName == "i")
       continue;
     if (IgnoreUnknown && !isSupportedExtension(ExtName))
       continue;



More information about the cfe-commits mailing list