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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:194
 
-  if (MArch.startswith_insensitive("rv32")) {
-    // FIXME: parse `March` to find `D` extension properly
-    if (MArch.substr(4).contains_insensitive("d") ||
-        MArch.startswith_insensitive("rv32g"))
-      return "ilp32d";
-    else if (MArch.startswith_insensitive("rv32e"))
-      return "ilp32e";
-    else
-      return "ilp32";
-  } else if (MArch.startswith_insensitive("rv64")) {
-    // FIXME: parse `March` to find `D` extension properly
-    if (MArch.substr(4).contains_insensitive("d") ||
-        MArch.startswith_insensitive("rv64g"))
-      return "lp64d";
-    else
-      return "lp64";
+  auto ParseResult = llvm::RISCVISAInfo::parseArchString(Arch, true);
+  if (!ParseResult) {
----------------
Please add an inline comment for what "true" represents.


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:206
+        return "ilp32d";
+      else if (HasE)
+        return "ilp32e";
----------------
Drop else after return


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:208
+        return "ilp32e";
+      else
+        return "ilp32";
----------------
here too


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:213
+        return "lp64d";
+      else
+        return "lp64";
----------------
Here too


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:216
+    }
   }
 
----------------
Should we have an llvm_unreachable here for unknown values of XLen?


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:43
+
+  /// OrderedExtensionMap is a StringMap-like container, but specialized to
+  /// keep entries in canonical order of extension.
----------------
I'm not sure the comparison to StringMap is helpful here. StringMap is an unordered map that stores strings in the same allocation as the value. This std::map is ordered and doesn't store the strings in the same allocation.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:286
+                                 bool ExperimentalExtensionVersionCheck) {
+  std::string MajorStr, MinorStr;
+  Major = 0;
----------------
Do MajorStr and MinorStr need to be std::strings or can they be StringRefs? Looks like they are either empty or a subset of In. So there shouldn't be a lifetime issue?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:352
+        Error += "." + MinorStr;
+      Error += " for experimental extension (this compiler supports " +
+               utostr(SupportedVers.Major) + "." + utostr(SupportedVers.Minor) +
----------------
Should this message say which extension the error applies to?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:388
+                            const std::vector<std::string> &Features) {
+  std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo());
+  assert(XLen == 32 || XLen == 64);
----------------
Use

```
auto ISAInfo = std::make_unique<RISCVISAInfo>()
```


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:390
+  assert(XLen == 32 || XLen == 64);
+  ISAInfo->XLen = XLen;
+
----------------
Can we make XLen part of the RISCVISAInfo constructor? Seems like we know that early enough.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:440
+                              bool ExperimentalExtensionVersionCheck) {
+  std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo());
+  // RISC-V ISA strings must be lowercase.
----------------
Can we delay constructing this until after the first 2 error checks? Then we could pass XLen to the constructor.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:709
+
+  if (XLen == 32)
+    Arch << "rv32";
----------------
Would this better as

```
Arch << "rv" << XLen;
```

That makes it future proof to a hypothetical rv128 someday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168