[PATCH] D44189: [RISCV] Verify the input value of -march=

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

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.



================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:34
+
+    // The canonical order specified in ISA manual.
+    StringRef StdExts = "mafdc";
----------------
I'd reference Table 22.1 in RISC-V User-Level ISA V2.2 for anyone who wants to verify this.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:37-38
+
+    bool hasF = false, hasD = false;
+    char baseline = MArch[4];
+
----------------
Should be HasF, HasD, and Baseline to conform to standard LLVM naming conventions.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:65
+    for (char c : Exts) {
+      // Check march is satisfied the canonical order.
+      while (StdExtsItr != StdExts.end() && *StdExtsItr != c)
----------------
I'd phrase this as "Check ISA extensions are specified in the canonical order."


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:99
+
+    // Dependency check
+    if (hasD && !hasF)
----------------
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".


https://reviews.llvm.org/D44189