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

[llvm][RISCV] Improve error message for invalid extension letters #90468

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

DavidSpickett
Copy link
Collaborator

Previously you got:
clang: error: invalid arch name 'rv64v', first letter should be 'e', 'i' or 'g'

Which to me, unfamiliar with riscv, reads as if I should have used "[eig]rv64v". Which is not what clang means.

Include the first bit in the error message to make this clearer:
clang: error: invalid arch name 'rv64v', first letter after 'rv64' should be 'e', 'i' or 'g'

Previously you got:
clang: error: invalid arch name 'rv64v', first letter should be 'e', 'i' or 'g'

Which to me, unfamiliar with riscv, reads as if I should have used
"[eig]rv64v". Which is not what clang means.

Include the first bit in the error message to make this clearer:
clang: error: invalid arch name 'rv64v', first letter after 'rv64' should be 'e', 'i' or 'g'
@DavidSpickett DavidSpickett requested a review from asb April 29, 2024 13:17
@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' labels Apr 29, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 29, 2024

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

@llvm/pr-subscribers-clang

Author: David Spickett (DavidSpickett)

Changes

Previously you got:
clang: error: invalid arch name 'rv64v', first letter should be 'e', 'i' or 'g'

Which to me, unfamiliar with riscv, reads as if I should have used "[eig]rv64v". Which is not what clang means.

Include the first bit in the error message to make this clearer:
clang: error: invalid arch name 'rv64v', first letter after 'rv64' should be 'e', 'i' or 'g'


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

3 Files Affected:

  • (modified) clang/test/Driver/riscv-arch.c (+3-3)
  • (modified) llvm/lib/TargetParser/RISCVISAInfo.cpp (+2-1)
  • (modified) llvm/unittests/TargetParser/RISCVISAInfoTest.cpp (+7-3)
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index 8399b4e97f86d5..abbe8612b3780a 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -209,7 +209,7 @@
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32q -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-LETTER %s
 // RV32-LETTER: error: invalid arch name 'rv32q',
-// RV32-LETTER: first letter should be 'e', 'i' or 'g'
+// RV32-LETTER: first letter after 'rv32' should be 'e', 'i' or 'g'
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32imcq -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ORDER %s
@@ -239,12 +239,12 @@
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32xabc -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32X %s
 // RV32X: error: invalid arch name 'rv32xabc',
-// RV32X: first letter should be 'e', 'i' or 'g'
+// RV32X: first letter after 'rv32' should be 'e', 'i' or 'g'
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32sabc -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32S %s
 // RV32S: error: invalid arch name 'rv32sabc',
-// RV32S: first letter should be 'e', 'i' or 'g'
+// RV32S: first letter after 'rv32' should be 'e', 'i' or 'g'
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32ix -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32X-NAME %s
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index 494dc76a18521c..20182fb06037c2 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -639,7 +639,8 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
   switch (Baseline) {
   default:
     return createStringError(errc::invalid_argument,
-                             "first letter should be 'e', 'i' or 'g'");
+                             "first letter after \'" + Arch.slice(0, 4) +
+                                 "\' should be 'e', 'i' or 'g'");
   case 'e':
   case 'i':
     break;
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index c945c4fbcf6352..9f23000d733d06 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -120,10 +120,14 @@ TEST(ParseArchString, RejectsInvalidBaseISA) {
     EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
               "string must begin with rv32{i,e,g} or rv64{i,e,g}");
   }
-  for (StringRef Input : {"rv32j", "rv64k", "rv32_i"}) {
+
+  for (StringRef Input : {"rv32j", "rv32_i"}) {
     EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
-              "first letter should be 'e', 'i' or 'g'");
+              "first letter after 'rv32' should be 'e', 'i' or 'g'");
   }
+
+  EXPECT_EQ(toString(RISCVISAInfo::parseArchString("rv64k", true).takeError()),
+            "first letter after 'rv64' should be 'e', 'i' or 'g'");
 }
 
 TEST(ParseArchString, RejectsUnsupportedBaseISA) {
@@ -395,7 +399,7 @@ TEST(ParseArchString, AcceptsAmbiguousFromRelaxExtensions) {
 TEST(ParseArchString, RejectsRelaxExtensionsNotStartWithEorIorG) {
   EXPECT_EQ(
       toString(RISCVISAInfo::parseArchString("rv32zba_im", true).takeError()),
-      "first letter should be 'e', 'i' or 'g'");
+      "first letter after 'rv32' should be 'e', 'i' or 'g'");
 }
 
 TEST(ParseArchString,

@DavidSpickett DavidSpickett changed the title [llvm][RISC-V] Improve error message for invalid extension letters [llvm][RISCV] Improve error message for invalid extension letters Apr 29, 2024
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, thanks.

@DavidSpickett DavidSpickett merged commit 6f02120 into llvm:main Apr 29, 2024
7 of 8 checks passed
@DavidSpickett DavidSpickett deleted the riscv-err branch April 29, 2024 15:51
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants