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

[Kito Cheng via Phabricator via llvm-commits]

kito-cheng added inline comments.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:328-330
+    for (const auto &Feature : FeatureVector) {
+      Features.AddFeature(Feature);
     }
----------------
jhenderson wrote:
> 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.
> What does AddFeature take?  Should Feature be a StringRef?

StringRef, that should take StringRef and take care about the lifetime in my first impression, but I realized that actually store in a std::vector<std::string>[1], so I just pass std::string here.

[1] https://llvm.org/doxygen/SubtargetFeature_8h_source.html#l00184



================
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;) {
----------------
jhenderson wrote:
> 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.
That's part RISC-V standard, we called it canonical order of arch. string[1], but that remind me I should refactor that to using `isSupportedExtension` rather than maintain a magical string here.

[1] https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L182


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125204