Skip to content

[RISCV] Improve error message when the extension is not supported #83989

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 2 commits into from
Mar 6, 2024

Conversation

4vtomat
Copy link
Member

@4vtomat 4vtomat commented Mar 5, 2024

If the "march" has some extension with version that is not supported, it
returns the error message like: "error: invalid arch name 'some_arch',
unsupported version number 2.0 for extension 'some_arch'", which is not
precise enough, it should return the message that only tells users "the
extension is not supported".

If the "march" has some extension with version that is not supported, it
returns the error message like: "error: invalid arch name 'some_arch',
unsupported version number 2.0 for extension 'some_arch'", which is not
precise enough, it should return the message that only tells users "the
extension is not supported".
@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' llvm:support labels Mar 5, 2024
@4vtomat 4vtomat requested a review from topperc March 5, 2024 10:39
@llvmbot
Copy link
Member

llvmbot commented Mar 5, 2024

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

@llvm/pr-subscribers-llvm-support

Author: Brandon Wu (4vtomat)

Changes

If the "march" has some extension with version that is not supported, it
returns the error message like: "error: invalid arch name 'some_arch',
unsupported version number 2.0 for extension 'some_arch'", which is not
precise enough, it should return the message that only tells users "the
extension is not supported".


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

2 Files Affected:

  • (modified) clang/test/Driver/riscv-arch.c (+2-2)
  • (modified) llvm/lib/Support/RISCVISAInfo.cpp (+16-10)
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index c9e984e07cbea9..8399b4e97f86d5 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -306,7 +306,7 @@
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32ist2p0 -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-SMINOR0 %s
 // RV32-SMINOR0: error: invalid arch name 'rv32ist2p0',
-// RV32-SMINOR0: unsupported version number 2.0 for extension 'st'
+// RV32-SMINOR0: unsupported standard supervisor-level extension 'st'
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_ -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XSEP %s
@@ -397,7 +397,7 @@
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32izbb1p0zbs1p0 -menable-experimental-extensions -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-EXPERIMENTAL-ZBB-ZBS-UNDERSCORE %s
-// RV32-EXPERIMENTAL-ZBB-ZBS-UNDERSCORE: error: invalid arch name 'rv32izbb1p0zbs1p0', unsupported version number 1.0 for extension 'zbb1p0zbs'
+// RV32-EXPERIMENTAL-ZBB-ZBS-UNDERSCORE: error: invalid arch name 'rv32izbb1p0zbs1p0', unsupported standard user-level extension 'zbb1p0zbs'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32izba1p0 -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ZBA %s
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 54b4dcb22de8e0..6eec03fd6f7082 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -531,6 +531,17 @@ std::vector<std::string> RISCVISAInfo::toFeatures(bool AddAllExtensions,
   return Features;
 }
 
+static Error getStringErrorForInvalidExt(std::string_view ExtName) {
+  if (ExtName.size() == 1) {
+    return createStringError(errc::invalid_argument,
+                             "unsupported standard user-level extension '" +
+                                 ExtName + "'");
+  }
+  return createStringError(errc::invalid_argument,
+                           "unsupported " + getExtensionTypeDesc(ExtName) +
+                               " '" + ExtName + "'");
+}
+
 // Extensions may have a version number, and may be separated by
 // an underscore '_' e.g.: rv32i2_m2.
 // Version number is divided into major and minor version numbers,
@@ -629,6 +640,9 @@ static Error getExtensionVersion(StringRef Ext, StringRef In, unsigned &Major,
   if (RISCVISAInfo::isSupportedExtension(Ext, Major, Minor))
     return Error::success();
 
+  if (!RISCVISAInfo::isSupportedExtension(Ext))
+    return getStringErrorForInvalidExt(Ext);
+
   std::string Error = "unsupported version number " + std::string(MajorStr);
   if (!MinorStr.empty())
     Error += "." + MinorStr.str();
@@ -965,16 +979,8 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
     const std::string &ExtName = SeenExtAndVers.first;
     RISCVISAInfo::ExtensionVersion ExtVers = SeenExtAndVers.second;
 
-    if (!RISCVISAInfo::isSupportedExtension(ExtName)) {
-      if (ExtName.size() == 1) {
-        return createStringError(errc::invalid_argument,
-                                 "unsupported standard user-level extension '" +
-                                     ExtName + "'");
-      }
-      return createStringError(errc::invalid_argument,
-                               "unsupported " + getExtensionTypeDesc(ExtName) +
-                                   " '" + ExtName + "'");
-    }
+    if (!RISCVISAInfo::isSupportedExtension(ExtName))
+      return getStringErrorForInvalidExt(ExtName);
     ISAInfo->addExtension(ExtName, ExtVers);
   }
 

@llvmbot
Copy link
Member

llvmbot commented Mar 5, 2024

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

Author: Brandon Wu (4vtomat)

Changes

If the "march" has some extension with version that is not supported, it
returns the error message like: "error: invalid arch name 'some_arch',
unsupported version number 2.0 for extension 'some_arch'", which is not
precise enough, it should return the message that only tells users "the
extension is not supported".


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

2 Files Affected:

  • (modified) clang/test/Driver/riscv-arch.c (+2-2)
  • (modified) llvm/lib/Support/RISCVISAInfo.cpp (+16-10)
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index c9e984e07cbea9..8399b4e97f86d5 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -306,7 +306,7 @@
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32ist2p0 -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-SMINOR0 %s
 // RV32-SMINOR0: error: invalid arch name 'rv32ist2p0',
-// RV32-SMINOR0: unsupported version number 2.0 for extension 'st'
+// RV32-SMINOR0: unsupported standard supervisor-level extension 'st'
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_ -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XSEP %s
@@ -397,7 +397,7 @@
 
 // RUN: not %clang --target=riscv32-unknown-elf -march=rv32izbb1p0zbs1p0 -menable-experimental-extensions -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-EXPERIMENTAL-ZBB-ZBS-UNDERSCORE %s
-// RV32-EXPERIMENTAL-ZBB-ZBS-UNDERSCORE: error: invalid arch name 'rv32izbb1p0zbs1p0', unsupported version number 1.0 for extension 'zbb1p0zbs'
+// RV32-EXPERIMENTAL-ZBB-ZBS-UNDERSCORE: error: invalid arch name 'rv32izbb1p0zbs1p0', unsupported standard user-level extension 'zbb1p0zbs'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32izba1p0 -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ZBA %s
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 54b4dcb22de8e0..6eec03fd6f7082 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -531,6 +531,17 @@ std::vector<std::string> RISCVISAInfo::toFeatures(bool AddAllExtensions,
   return Features;
 }
 
+static Error getStringErrorForInvalidExt(std::string_view ExtName) {
+  if (ExtName.size() == 1) {
+    return createStringError(errc::invalid_argument,
+                             "unsupported standard user-level extension '" +
+                                 ExtName + "'");
+  }
+  return createStringError(errc::invalid_argument,
+                           "unsupported " + getExtensionTypeDesc(ExtName) +
+                               " '" + ExtName + "'");
+}
+
 // Extensions may have a version number, and may be separated by
 // an underscore '_' e.g.: rv32i2_m2.
 // Version number is divided into major and minor version numbers,
@@ -629,6 +640,9 @@ static Error getExtensionVersion(StringRef Ext, StringRef In, unsigned &Major,
   if (RISCVISAInfo::isSupportedExtension(Ext, Major, Minor))
     return Error::success();
 
+  if (!RISCVISAInfo::isSupportedExtension(Ext))
+    return getStringErrorForInvalidExt(Ext);
+
   std::string Error = "unsupported version number " + std::string(MajorStr);
   if (!MinorStr.empty())
     Error += "." + MinorStr.str();
@@ -965,16 +979,8 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
     const std::string &ExtName = SeenExtAndVers.first;
     RISCVISAInfo::ExtensionVersion ExtVers = SeenExtAndVers.second;
 
-    if (!RISCVISAInfo::isSupportedExtension(ExtName)) {
-      if (ExtName.size() == 1) {
-        return createStringError(errc::invalid_argument,
-                                 "unsupported standard user-level extension '" +
-                                     ExtName + "'");
-      }
-      return createStringError(errc::invalid_argument,
-                               "unsupported " + getExtensionTypeDesc(ExtName) +
-                                   " '" + ExtName + "'");
-    }
+    if (!RISCVISAInfo::isSupportedExtension(ExtName))
+      return getStringErrorForInvalidExt(ExtName);
     ISAInfo->addExtension(ExtName, ExtVers);
   }
 

@4vtomat 4vtomat requested review from kito-cheng, BeMg and asb March 5, 2024 10:39
Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@dwblaikie
Copy link
Collaborator

perhaps the llvm libSupport prats of this change should be unit tested in LLVM, rather than only tested indirectly in clang?

@4vtomat
Copy link
Member Author

4vtomat commented Mar 6, 2024

perhaps the llvm libSupport prats of this change should be unit tested in LLVM, rather than only tested indirectly in clang?

Good point, thanks!
I've added for unites.

@4vtomat 4vtomat merged commit 11f74cd into llvm:main Mar 6, 2024
@4vtomat 4vtomat deleted the improve_error_msg branch March 6, 2024 01:25
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 llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants