Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV] Deduplicate RISCVISAInfo::toFeatures/toFeatureVector. NFC #76942

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Jan 4, 2024

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.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:support llvm:binary-utilities labels Jan 4, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-clang

Author: Luke Lau (lukel97)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/76942.diff

6 Files Affected:

  • (modified) clang/lib/Basic/Targets/RISCV.cpp (+3-3)
  • (modified) clang/lib/Driver/ToolChains/Arch/RISCV.cpp (+3-3)
  • (modified) llvm/include/llvm/Support/RISCVISAInfo.h (+2-4)
  • (modified) llvm/lib/Object/ELFObjectFile.cpp (+1-1)
  • (modified) llvm/lib/Support/RISCVISAInfo.cpp (+12-26)
  • (modified) llvm/unittests/Support/RISCVISAInfoTest.cpp (+25-5)
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) {

clang/lib/Driver/ToolChains/Arch/RISCV.cpp Outdated Show resolved Hide resolved
llvm/lib/Support/RISCVISAInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Support/RISCVISAInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/Support/RISCVISAInfo.cpp Outdated Show resolved Hide resolved
bool IgnoreUnknown) const {
std::vector<std::string> Features;
for (const auto &[ExtName, _] : Exts) {
if (ExtName == "i") // i is not recognized in clang -cc1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment explaining that this is the RVI i extension with a link to the spec? I fear that non-RISCV developers might be confused by the random check for i.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just copied from the previous function, but would non-RISCV developers need to read RISCVISAInfo.cpp? (Still happy to add a link to the spec if you would prefer)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sometimes changes do require looking at other architectures.

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.
@lukel97 lukel97 force-pushed the deduplicate-riscvisainfo-tofeatures branch from fa12f6b to 18d0666 Compare January 9, 2024 06:50
@lukel97 lukel97 merged commit db78c30 into llvm:main Jan 9, 2024
4 checks passed
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…vm#76942)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category llvm:binary-utilities llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants