[RISCV] Verify the input value of -march=

[kito-cheng]

Summary

This patch doing more check and verify the -march= string and will issue an error if it's a invalid combination.

[apazos]

lib/Driver/ToolChains/Arch/RISCV.cpp

49

In the switch cases move default to first position.

88

So the subsequent features can appear in any order?

[kito-cheng]

lib/Driver/ToolChains/Arch/RISCV.cpp

49

Done :)

88

Yeah, here is a canonical order specified in ISA manual, I've check the order now.

[kito-cheng]

This version only update variable name which changed in last version by accident.

[kito-cheng]

Add test cases for the correct inputs.

[asb]

Thanks for submitting this Kito. I've added some minor in-line comments. It might also be worth adding a couple of extra cases to the tests:

• Repeated letters in the ISA string (e.g. rv32immafd)
• Upper case letters in the ISA string. We currently reject these (as does GCC). It would be worth having a test that tracks this behaviour

We could probably give more informative error messages than just "invalid arch name". Especially for common errors like -march=rv32. If not adding such diagnostics in this patch, it would be good to add a // TODO note to record that we'd like to do better.

lib/Driver/ToolChains/Arch/RISCV.cpp

35

I'd reference Table 22.1 in RISC-V User-Level ISA V2.2 for anyone who wants to verify this.

38–39

Should be HasF, HasD, and Baseline to conform to standard LLVM naming conventions.

40

I'd phrase this as "Check ISA extensions are specified in the canonical order."

107

I'd be tempted to give a bit more explanation a bit more "It's illegal to specify the 'd' (double-precision floating point) extension without also specifying the 'f' (single precision floating-point) extension".

[kito-cheng]

Update revision according Alex's review.

Changes:

• Add testcase for uppercase of -march string.
• Add testcase for repeated letter in -march.
• Add more comment.
• Add several TODO item for diagnostic message improvement.
• Fix coding style issue.

[kito-cheng]

Hi Alex:

Thanks for your input, check for repeated letter was missed in my last patch :)

lib/Driver/ToolChains/Arch/RISCV.cpp

35

Done.

38–39

Fixed.

40

Done.

107

Done.

[asb]

Thanks for this Kito. A tiny formatting nit, but otherwise this looks good to me.

lib/Driver/ToolChains/Arch/RISCV.cpp

38

Indent should be 2 spaces rather than 3.

[apazos]

lib/Driver/ToolChains/Arch/RISCV.cpp

51

One more question - how about non-standard extensions (vendor/custom) prefixed with X?
Shouldn't we add the logic to process 'Xext' occurrences in the march string as part of this patch?

lib/Driver/ToolChains/Arch/RISCV.cpp

 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 void riscv::getRISCVTargetFeatures(const Driver &D, const ArgList &Args,
                                    std::vector<StringRef> &Features) {
   if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
     StringRef MArch = A->getValue();
-    // TODO: handle rv64
-    std::pair<StringRef, StringRef> MArchSplit = StringRef(MArch).split("rv32");
-    if (!MArchSplit.second.size())
+    if (!(MArch.startswith("rv32") || MArch.startswith("rv64")) ||
+        (MArch.size() < 5)) {
+      // ISA string must begin with rv32 or rv64.
+      // TODO: Improve diagnostic message.
+      D.Diag(diag::err_drv_invalid_arch_name) << MArch;
       return;
+    }
 
-    for (char c : MArchSplit.second) {
-      switch (c) {
-      case 'i':
-        break;
+    // The canonical order specified in ISA manual.
+    // Ref: Table 22.1 in RISC-V User-Level ISA V2.2
+    StringRef StdExts = "mafdc";
+
+    bool HasF = false, HasD = false;
+    char Baseline = MArch[4];
+
+    // TODO: Add 'e' once backend supported.
+    switch (Baseline) {
+    default:
+      // First letter should be 'e', 'i' or 'g'.
+      // TODO: Improve diagnostic message.
+      D.Diag(diag::err_drv_invalid_arch_name) << MArch;
+      return;
+    case 'g':
+      // g = imafd
+      StdExts = StdExts.drop_front(4);
+      Features.push_back("+m");
+      Features.push_back("+a");
+      Features.push_back("+f");
+      Features.push_back("+d");
+      HasF = true;
+      HasD = true;
+      break;
+    }
+
+    auto StdExtsItr = StdExts.begin();
+    // Skip rvxxx
+    StringRef Exts = MArch.substr(5);
+
+    for (char c : Exts) {
+      // Check ISA extensions are specified in the canonical order.
+      while (StdExtsItr != StdExts.end() && *StdExtsItr != c)
+        ++StdExtsItr;
+
+      if (StdExtsItr == StdExts.end()) {
+        // TODO: Improve diagnostic message.
+        D.Diag(diag::err_drv_invalid_arch_name) << MArch;
+        return;
+      }
+
+      // Move to next char to prevent repeated letter.
+      ++StdExtsItr;
+
+      // The order is OK, then push it into features.
+      switch (c) {
+      default:
+        // TODO: Improve diagnostic message.
+        D.Diag(diag::err_drv_invalid_arch_name) << MArch;
+        return;
       case 'm':
         Features.push_back("+m");
         break;
       case 'a':
         Features.push_back("+a");
         break;
       case 'f':
         Features.push_back("+f");
+        HasF = true;
         break;
       case 'd':
         Features.push_back("+d");
+        HasD = true;
         break;
       case 'c':
         Features.push_back("+c");
         break;
       }
     }
+
+    // Dependency check
+    // It's illegal to specify the 'd' (double-precision floating point)
+    // extension without also specifying the 'f' (single precision
+    // floating-point) extension.
+    // TODO: Improve diagnostic message.
+    if (HasD && !HasF)
+      D.Diag(diag::err_drv_invalid_arch_name) << MArch;
   }
 }
 
 StringRef riscv::getRISCVABI(const ArgList &Args, const llvm::Triple &Triple) {
   if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ))
     return A->getValue();
 
   return Triple.getArch() == llvm::Triple::riscv32 ? "ilp32" : "lp64";
 }

test/Driver/riscv-arch.c

+// RUN: %clang -target riscv32-unknown-elf -march=rv32i -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32im -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32ima -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32ic -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32ia -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32g -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32gc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64i -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64im -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64ima -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64ic -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64ia -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64g -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64gc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// CHECK-NOT: error: invalid arch name '
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32 -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32 %s
+// RV32: error: invalid arch name 'rv32'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32m -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32M %s
+// RV32M: error: invalid arch name 'rv32m'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32id -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32ID %s
+// RV32ID: error: invalid arch name 'rv32id'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32l -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32L %s
+// RV32L: error: invalid arch name 'rv32l'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imadf -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32IMADF %s
+// RV32IMADF: error: invalid arch name 'rv32imadf'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imm -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32IMM %s
+// RV32IMM: error: invalid arch name 'rv32imm'
+
+// RUN: %clang -target riscv32-unknown-elf -march=RV32I -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32I-UPPER %s
+// RV32I-UPPER: error: invalid arch name 'RV32I'
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64 -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64 %s
+// RV64: error: invalid arch name 'rv64'
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64m -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64M %s
+// RV64M: error: invalid arch name 'rv64m'
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64id -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64ID %s
+// RV64ID: error: invalid arch name 'rv64id'
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64l -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64L %s
+// RV64L: error: invalid arch name 'rv64l'
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imadf -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64IMADF %s
+// RV64IMADF: error: invalid arch name 'rv64imadf'
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imm -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64IMM %s
+// RV64IMM: error: invalid arch name 'rv64imm'
+
+// RUN: %clang -target riscv64-unknown-elf -march=RV64I -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64I-UPPER %s
+// RV64I-UPPER: error: invalid arch name 'RV64I'