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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
