[PATCH] D125204: [RISCV] Use RISCVISAInfo to parse arch string from ELF build attribute.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 9 01:28:36 PDT 2022
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
More information about the llvm-commits
mailing list