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] Refactor profile selection in RISCVISAInfo::parseArchString. #90700

Merged
merged 1 commit into from
May 1, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented May 1, 2024

Instead of hardcoding the 4 current profile prefixes, treat profile selection as a fallback if we don't find "rv32" or "rv64".

Update the error message accordingly.

Instead of hardcoding the 4 current profile prefixes, treat profile
selection as a fallback if we don't find "rv32" or "rv64".

Update the error message accordingly.
@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' mc Machine (object) code labels May 1, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 1, 2024

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-driver

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

Author: Craig Topper (topperc)

Changes

Instead of hardcoding the 4 current profile prefixes, treat profile selection as a fallback if we don't find "rv32" or "rv64".

Update the error message accordingly.


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

5 Files Affected:

  • (modified) clang/test/Driver/riscv-arch.c (+1-1)
  • (modified) clang/test/Driver/riscv-profiles.c (+1-1)
  • (modified) llvm/lib/TargetParser/RISCVISAInfo.cpp (+22-23)
  • (modified) llvm/test/MC/RISCV/invalid-attribute.s (+1-1)
  • (modified) llvm/unittests/TargetParser/RISCVISAInfoTest.cpp (+4-2)
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index abbe8612b3780a..8c701a736fc7e0 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -204,7 +204,7 @@
 // RUN: not %clang --target=riscv32-unknown-elf -march=unknown -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-STR %s
 // RV32-STR: error: invalid arch name 'unknown',
-// RV32-STR: string must begin with rv32{i,e,g} or rv64{i,e,g}
+// RV32-STR: string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported profile name
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32q -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-LETTER %s
diff --git a/clang/test/Driver/riscv-profiles.c b/clang/test/Driver/riscv-profiles.c
index 647567d4c971f4..298f301de3feb6 100644
--- a/clang/test/Driver/riscv-profiles.c
+++ b/clang/test/Driver/riscv-profiles.c
@@ -318,7 +318,7 @@
 // PROFILE-WITH-ADDITIONAL: "-target-feature" "+zkt"
 
 // RUN: not %clang --target=riscv64 -### -c %s 2>&1 -march=rva19u64_zfa | FileCheck -check-prefix=INVALID-PROFILE %s
-// INVALID-PROFILE: error: invalid arch name 'rva19u64_zfa', unsupported profile
+// INVALID-PROFILE: error: invalid arch name 'rva19u64_zfa', string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported profile name
 
 // RUN: not %clang --target=riscv64 -### -c %s 2>&1 -march=rva22u64zfa | FileCheck -check-prefix=INVALID-ADDITIONAL %s
 // INVALID-ADDITIONAL: error: invalid arch name 'rva22u64zfa', additional extensions must be after separator '_'
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index 3b0cf8fab25f46..d154c00a785927 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -592,40 +592,39 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
     return createStringError(errc::invalid_argument,
                              "string must be lowercase");
 
-  if (Arch.starts_with("rvi") || Arch.starts_with("rva") ||
-      Arch.starts_with("rvb") || Arch.starts_with("rvm")) {
+  // ISA string must begin with rv32, rv64, or a profile.
+  unsigned XLen = 0;
+  if (Arch.consume_front("rv32")) {
+    XLen = 32;
+  } else if (Arch.consume_front("rv64")) {
+    XLen = 64;
+  } else {
+    // Try parsing as a profile.
     const auto *FoundProfile =
         llvm::find_if(SupportedProfiles, [Arch](const RISCVProfile &Profile) {
           return Arch.starts_with(Profile.Name);
         });
 
-    if (FoundProfile == std::end(SupportedProfiles))
-      return createStringError(errc::invalid_argument, "unsupported profile");
-
-    std::string NewArch = FoundProfile->MArch.str();
-    StringRef ArchWithoutProfile = Arch.substr(FoundProfile->Name.size());
-    if (!ArchWithoutProfile.empty()) {
-      if (!ArchWithoutProfile.starts_with("_"))
-        return createStringError(
-            errc::invalid_argument,
-            "additional extensions must be after separator '_'");
-      NewArch += ArchWithoutProfile.str();
+    if (FoundProfile != std::end(SupportedProfiles)) {
+      std::string NewArch = FoundProfile->MArch.str();
+      StringRef ArchWithoutProfile = Arch.drop_front(FoundProfile->Name.size());
+      if (!ArchWithoutProfile.empty()) {
+        if (ArchWithoutProfile.front() != '_')
+          return createStringError(
+              errc::invalid_argument,
+              "additional extensions must be after separator '_'");
+        NewArch += ArchWithoutProfile.str();
+      }
+      return parseArchString(NewArch, EnableExperimentalExtension,
+                             ExperimentalExtensionVersionCheck, IgnoreUnknown);
     }
-    return parseArchString(NewArch, EnableExperimentalExtension,
-                           ExperimentalExtensionVersionCheck, IgnoreUnknown);
   }
 
-  // ISA string must begin with rv32 or rv64.
-  unsigned XLen = 0;
-  if (Arch.consume_front("rv32"))
-    XLen = 32;
-  else if (Arch.consume_front("rv64"))
-    XLen = 64;
-
   if (XLen == 0 || Arch.empty())
     return createStringError(
         errc::invalid_argument,
-        "string must begin with rv32{i,e,g} or rv64{i,e,g}");
+        "string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported "
+        "profile name");
 
   std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
   MapVector<std::string, RISCVISAUtils::ExtensionVersion,
diff --git a/llvm/test/MC/RISCV/invalid-attribute.s b/llvm/test/MC/RISCV/invalid-attribute.s
index 1d732af83cda35..2989e80b269ae0 100644
--- a/llvm/test/MC/RISCV/invalid-attribute.s
+++ b/llvm/test/MC/RISCV/invalid-attribute.s
@@ -11,7 +11,7 @@
 # CHECK: [[@LINE-1]]:12: error: attribute name not recognised: unknown
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,e,g}
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported profile name{{$}}
 
 .attribute arch, "rv32i2p1_y2p0"
 # CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p1_y2p0', invalid standard user-level extension 'y'
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index 9f23000d733d06..3aa0178100abf4 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -118,7 +118,8 @@ TEST(ParseArchString, RejectsUpperCase) {
 TEST(ParseArchString, RejectsInvalidBaseISA) {
   for (StringRef Input : {"rv32", "rv64", "rv65i"}) {
     EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
-              "string must begin with rv32{i,e,g} or rv64{i,e,g}");
+              "string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported "
+              "profile name");
   }
 
   for (StringRef Input : {"rv32j", "rv32_i"}) {
@@ -133,7 +134,8 @@ TEST(ParseArchString, RejectsInvalidBaseISA) {
 TEST(ParseArchString, RejectsUnsupportedBaseISA) {
   for (StringRef Input : {"rv128i", "rv128g"}) {
     EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
-              "string must begin with rv32{i,e,g} or rv64{i,e,g}");
+              "string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported "
+              "profile name");
   }
 }
 

@llvmbot
Copy link
Collaborator

llvmbot commented May 1, 2024

@llvm/pr-subscribers-mc

Author: Craig Topper (topperc)

Changes

Instead of hardcoding the 4 current profile prefixes, treat profile selection as a fallback if we don't find "rv32" or "rv64".

Update the error message accordingly.


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

5 Files Affected:

  • (modified) clang/test/Driver/riscv-arch.c (+1-1)
  • (modified) clang/test/Driver/riscv-profiles.c (+1-1)
  • (modified) llvm/lib/TargetParser/RISCVISAInfo.cpp (+22-23)
  • (modified) llvm/test/MC/RISCV/invalid-attribute.s (+1-1)
  • (modified) llvm/unittests/TargetParser/RISCVISAInfoTest.cpp (+4-2)
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index abbe8612b3780a..8c701a736fc7e0 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -204,7 +204,7 @@
 // RUN: not %clang --target=riscv32-unknown-elf -march=unknown -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-STR %s
 // RV32-STR: error: invalid arch name 'unknown',
-// RV32-STR: string must begin with rv32{i,e,g} or rv64{i,e,g}
+// RV32-STR: string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported profile name
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32q -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-LETTER %s
diff --git a/clang/test/Driver/riscv-profiles.c b/clang/test/Driver/riscv-profiles.c
index 647567d4c971f4..298f301de3feb6 100644
--- a/clang/test/Driver/riscv-profiles.c
+++ b/clang/test/Driver/riscv-profiles.c
@@ -318,7 +318,7 @@
 // PROFILE-WITH-ADDITIONAL: "-target-feature" "+zkt"
 
 // RUN: not %clang --target=riscv64 -### -c %s 2>&1 -march=rva19u64_zfa | FileCheck -check-prefix=INVALID-PROFILE %s
-// INVALID-PROFILE: error: invalid arch name 'rva19u64_zfa', unsupported profile
+// INVALID-PROFILE: error: invalid arch name 'rva19u64_zfa', string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported profile name
 
 // RUN: not %clang --target=riscv64 -### -c %s 2>&1 -march=rva22u64zfa | FileCheck -check-prefix=INVALID-ADDITIONAL %s
 // INVALID-ADDITIONAL: error: invalid arch name 'rva22u64zfa', additional extensions must be after separator '_'
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index 3b0cf8fab25f46..d154c00a785927 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -592,40 +592,39 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
     return createStringError(errc::invalid_argument,
                              "string must be lowercase");
 
-  if (Arch.starts_with("rvi") || Arch.starts_with("rva") ||
-      Arch.starts_with("rvb") || Arch.starts_with("rvm")) {
+  // ISA string must begin with rv32, rv64, or a profile.
+  unsigned XLen = 0;
+  if (Arch.consume_front("rv32")) {
+    XLen = 32;
+  } else if (Arch.consume_front("rv64")) {
+    XLen = 64;
+  } else {
+    // Try parsing as a profile.
     const auto *FoundProfile =
         llvm::find_if(SupportedProfiles, [Arch](const RISCVProfile &Profile) {
           return Arch.starts_with(Profile.Name);
         });
 
-    if (FoundProfile == std::end(SupportedProfiles))
-      return createStringError(errc::invalid_argument, "unsupported profile");
-
-    std::string NewArch = FoundProfile->MArch.str();
-    StringRef ArchWithoutProfile = Arch.substr(FoundProfile->Name.size());
-    if (!ArchWithoutProfile.empty()) {
-      if (!ArchWithoutProfile.starts_with("_"))
-        return createStringError(
-            errc::invalid_argument,
-            "additional extensions must be after separator '_'");
-      NewArch += ArchWithoutProfile.str();
+    if (FoundProfile != std::end(SupportedProfiles)) {
+      std::string NewArch = FoundProfile->MArch.str();
+      StringRef ArchWithoutProfile = Arch.drop_front(FoundProfile->Name.size());
+      if (!ArchWithoutProfile.empty()) {
+        if (ArchWithoutProfile.front() != '_')
+          return createStringError(
+              errc::invalid_argument,
+              "additional extensions must be after separator '_'");
+        NewArch += ArchWithoutProfile.str();
+      }
+      return parseArchString(NewArch, EnableExperimentalExtension,
+                             ExperimentalExtensionVersionCheck, IgnoreUnknown);
     }
-    return parseArchString(NewArch, EnableExperimentalExtension,
-                           ExperimentalExtensionVersionCheck, IgnoreUnknown);
   }
 
-  // ISA string must begin with rv32 or rv64.
-  unsigned XLen = 0;
-  if (Arch.consume_front("rv32"))
-    XLen = 32;
-  else if (Arch.consume_front("rv64"))
-    XLen = 64;
-
   if (XLen == 0 || Arch.empty())
     return createStringError(
         errc::invalid_argument,
-        "string must begin with rv32{i,e,g} or rv64{i,e,g}");
+        "string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported "
+        "profile name");
 
   std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
   MapVector<std::string, RISCVISAUtils::ExtensionVersion,
diff --git a/llvm/test/MC/RISCV/invalid-attribute.s b/llvm/test/MC/RISCV/invalid-attribute.s
index 1d732af83cda35..2989e80b269ae0 100644
--- a/llvm/test/MC/RISCV/invalid-attribute.s
+++ b/llvm/test/MC/RISCV/invalid-attribute.s
@@ -11,7 +11,7 @@
 # CHECK: [[@LINE-1]]:12: error: attribute name not recognised: unknown
 
 .attribute arch, "foo"
-# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g} or rv64{i,e,g}
+# CHECK: [[@LINE-1]]:18: error: invalid arch name 'foo', string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported profile name{{$}}
 
 .attribute arch, "rv32i2p1_y2p0"
 # CHECK: [[@LINE-1]]:18: error: invalid arch name 'rv32i2p1_y2p0', invalid standard user-level extension 'y'
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index 9f23000d733d06..3aa0178100abf4 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -118,7 +118,8 @@ TEST(ParseArchString, RejectsUpperCase) {
 TEST(ParseArchString, RejectsInvalidBaseISA) {
   for (StringRef Input : {"rv32", "rv64", "rv65i"}) {
     EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
-              "string must begin with rv32{i,e,g} or rv64{i,e,g}");
+              "string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported "
+              "profile name");
   }
 
   for (StringRef Input : {"rv32j", "rv32_i"}) {
@@ -133,7 +134,8 @@ TEST(ParseArchString, RejectsInvalidBaseISA) {
 TEST(ParseArchString, RejectsUnsupportedBaseISA) {
   for (StringRef Input : {"rv128i", "rv128g"}) {
     EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
-              "string must begin with rv32{i,e,g} or rv64{i,e,g}");
+              "string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported "
+              "profile name");
   }
 }
 

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 09f4b06 into llvm:main May 1, 2024
8 of 9 checks passed
@topperc topperc deleted the pr/profile-parsing branch May 1, 2024 17:39
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 Clang issues not falling into any other category mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants