[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.


================
Comment at: clang/test/Driver/riscv-abi.c:68
 
-// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64d -mabi=lp64d 2>&1 \
+// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64ifd -mabi=lp64d 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64D %s
----------------
khchen wrote:
> Why do we need to modify here?
rv64d is not a valid arch string. rv64id is though; would that work here? These tests are just currently testing behaviour that _should_ have been an error... (see also rv64 f above)


================
Comment at: clang/test/Driver/riscv-arch.c:198-201
-// RUN: %clang -target riscv32-unknown-elf -march=rv32e -### %s \
-// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32E %s
-// RV32E: error: invalid arch name 'rv32e',
-// RV32E: standard user-level extension 'e'
----------------
Hm, given we don't currently support RV32E properly this should probably be an error still, but could be a "nicer" error than a generic "invalid arch name" (which is technically wrong)


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:45
+
+  // Parse RISCV ISA info from arch string.
+  Error parse(StringRef Arch, bool EnableExperimentalExtension,
----------------
These comments aren't helpful. If you want to write full doxygen then you can, but a one-line comment that won't appear in documentation and is obvious from the function name and signature is just clutter.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:24
+namespace {
+/// Represents the major and version number components of a RISC-V extension
+struct RISCVExtensionVersion {
----------------
This seems unnecessary


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:32
+  const char *Name;
+  /// Supported version.
+  RISCVExtensionVersion Version;
----------------
Ditto for these


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+
----------------
craig.topper wrote:
> Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
I can't help but feel this is really an array of chars, not a string. We don't even need the trailing NUL, though double quote syntax is simpler than curly braces and a bunch of single-quote chars just to save a byte.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:43
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+    {"i", RISCVExtensionVersion{2, 0}, /* Experimental */ false},
+    {"e", RISCVExtensionVersion{1, 9}, /* Experimental */ false},
----------------
but that's the default so I'd omit it for anything other than the cases where it's true


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:71-72
+
+// Remove 'experimental-' if it's prefix of string,
+// return true if did the strip.
+static bool stripExperimentalPrefix(StringRef &Ext) {
----------------
Comment isn't useful, it's a trivial wrapper


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:77
+
+RISCVISAInfo::RISCVISAInfo() : XLen(0), FLen(0) {}
+
----------------
Can this not go in the header as a trivial constructor?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:112
+
+// Helper function for filtering SupportedExtensionInfos by name.
+static auto filterSupportedExtensionInfosByName(StringRef ExtName) {
----------------
Obvious comment


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:135
+  if (CheckExperimental)
+    IsExperimental = stripExperimentalPrefix(Ext);
+
----------------
*Extensions* don't have an experimental- prefix, *internal target features* do, so something strange is going on here. This feels like we're confusing several things:
1. Whether or not Ext is a feature or an extension
2. Whether or not Ext is experimental
3. Whether we want to look at experimental extensions
Some of those are somewhat necessarily interwoven, but the naming does not currently accurately reflect what these things mean, and I would argue we should be very explicit and keep features and extensions separate, never using the same thing to represent both.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:193
+    // order, but after all known standard extensions.
+    Rank = AllStdExts.size() + (Ext - 'a');
+  else
----------------
This is wrong as it doesn't include the +2 to account for I and E, though does happen to work because A and B are both known single-letter standard extensions so you can't actually get it to overlap (but were AllStdExts to omit one of them you would be able to).

IMO though it would be cleaner to use negative numbers for I and E, as they're special, and that means you can just start the counting at 0 for all the others, not needing the +2 anywhere.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:235-236
+
+// Compare function for extension.
+// Only compare the extension name, ignore version comparison.
+bool RISCVISAInfo::compareExtension(const std::string &LHS,
----------------
This belongs as a more fully-fledged doxgen comment in the header.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:453
+    StringRef Error;
+    // Currently LLVM does not support 'e'.
+    // Extension 'e' is not allowed in rv64.
----------------
Yet you removed the error for it?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:548
+            "unsupported standard user-level extension '%c'", C);
+      }
+    case 'm':
----------------



================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:595-596
+  // floating-point) extension.
+  // TODO: This has been removed in later spec, later spec has specify
+  //       D has implied F.
+  if (HasD && !HasF) {
----------------
(wrapped appropriately)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105168/new/

https://reviews.llvm.org/D105168