[PATCH] D125204: [RISCV] Use RISCVISAInfo to parse arch string from ELF build attribute.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:308-309
   if (Attr.hasValue()) {
-    // The Arch pattern is [rv32|rv64][i|e]version(_[m|a|f|d|c]version)*
-    // Version string pattern is (major)p(minor). Major and minor are optional.
-    // For example, a version number could be 2p0, 2, or p92.
-    StringRef Arch = Attr.getValue();
-    if (Arch.consume_front("rv32"))
+    // Suppress version checking for experimental extension to prevent that stop
+    // when got any unknown version.
+    auto ParseResult = RISCVISAInfo::parseArchString(
----------------



================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:311-312
+    auto ParseResult = RISCVISAInfo::parseArchString(
+        *Attr, /* EnableExperimentalExtension */ true,
+        /*ExperimentalExtensionVersionCheck*/ false, true);
+    if (!ParseResult) {
----------------
The final argument needs a comment too.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:328-330
+    for (const auto &Feature : FeatureVector) {
+      Features.AddFeature(Feature);
     }
----------------
What does `AddFeature` take? Should Feature be a `StringRef`? If not, I'd still spell out the type rather than use `auto`.

Also, no need for braces for single-line for loops.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:563
   auto StdExtsEnd = StdExts.end();
+  StringRef SupportedStandardExtension = "mafdcbv";
   for (auto I = Exts.begin(), E = Exts.end(); I != E;) {
----------------
I don't know `RISC-V` at all, but is there any particular reason this is in the order it is? If not, I'd suggest changing it to alphabetical order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125204