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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits