Skip to content

[RISCV] Don't pre-split before the loop in parseNormalizedArchString. #91684

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

Merged
merged 1 commit into from
May 10, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented May 10, 2024

We can extract each extension as we process them without much complexity.

I changed the error message for cases where there are double underscores or a trailing underscore. I think this is an improvement over the previous error.

We can extract each extension as we process them without much
complexity.

I changed the error message for cases where there are double
underscores or a trailing underscore. I think this is an improvement
over the previous error.
@llvmbot
Copy link
Member

llvmbot commented May 10, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

We can extract each extension as we process them without much complexity.

I changed the error message for cases where there are double underscores or a trailing underscore. I think this is an improvement over the previous error.


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

2 Files Affected:

  • (modified) llvm/lib/TargetParser/RISCVISAInfo.cpp (+12-3)
  • (modified) llvm/unittests/TargetParser/RISCVISAInfoTest.cpp (+7-2)
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index c553e330a878b..033df27c68781 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -451,9 +451,18 @@ RISCVISAInfo::parseNormalizedArchString(StringRef Arch) {
   // Each extension is of the form ${name}${major_version}p${minor_version}
   // and separated by _. Split by _ and then extract the name and version
   // information for each extension.
-  SmallVector<StringRef, 8> Split;
-  Arch.split(Split, '_');
-  for (StringRef Ext : Split) {
+  while (!Arch.empty()) {
+    if (Arch[0] == '_') {
+      if (Arch.size() == 1 || Arch[1] == '_')
+        return createStringError(errc::invalid_argument,
+                                 "extension name missing after separator '_'");
+      Arch = Arch.drop_front();
+    }
+
+    size_t Idx = Arch.find('_');
+    StringRef Ext = Arch.slice(0, Idx);
+    Arch = Arch.slice(Idx, StringRef::npos);
+
     StringRef Prefix, MinorVersionStr;
     std::tie(Prefix, MinorVersionStr) = Ext.rsplit('p');
     if (MinorVersionStr.empty())
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index f9e386a85fea8..7f2d1eb8c0170 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -38,12 +38,17 @@ TEST(ParseNormalizedArchString, RejectsInvalidBaseISA) {
 }
 
 TEST(ParseNormalizedArchString, RejectsMalformedInputs) {
-  for (StringRef Input :
-       {"rv64i2p0_", "rv32i2p0__a2p0", "rv64e2p", "rv32i", "rv64ip1"}) {
+  for (StringRef Input : {"rv64e2p", "rv32i", "rv64ip1"}) {
     EXPECT_EQ(
         toString(RISCVISAInfo::parseNormalizedArchString(Input).takeError()),
         "extension lacks version in expected format");
   }
+
+  for (StringRef Input : {"rv64i2p0_", "rv32i2p0__a2p0"}) {
+    EXPECT_EQ(
+        toString(RISCVISAInfo::parseNormalizedArchString(Input).takeError()),
+        "extension name missing after separator '_'");
+  }
 }
 
 TEST(ParseNormalizedArchString, RejectsOnlyVersion) {

@dtcxzyw dtcxzyw requested review from wangpc-pp and removed request for pcwang-thead May 10, 2024 00:11
Copy link
Contributor

@asb asb left a comment

Choose a reason for hiding this comment

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

LGTM

@topperc topperc merged commit d8f8ac8 into llvm:main May 10, 2024
@topperc topperc deleted the pr/presplit branch May 10, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants