[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.
Craig Topper via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 11 11:39:10 PDT 2021
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
More information about the llvm-commits
mailing list