asb added a comment.
I've added a couple of inline comments, but otherwise this seems fine to me.
I'd suggest updating the patch description to reference Philip's documentation
patch (which was posted soon after this), and also to explain why there are no
codegen changes (I think "Either changes to the specification required no
codegen changes, or LLVM was written against a newer version of the
specification than it claimed" is vague but accurate).
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:625
case 'g':
// g = imafd
if (Arch.size() > 5 && isdigit(Arch[5]))
----------------
Perhaps update to `// g expands to extensions in RISCVGImplications.` or delete
altogether.
================
Comment at: llvm/unittests/Support/RISCVISAInfoTest.cpp:253
TEST(ParseArchString, AcceptsVersionInLongOrShortForm) {
- for (StringRef Input : {"rv64i2", "rv64i2p0"}) {
- auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true);
----------------
Why is this test dropped? It's not redundant as there's a slightly separate
path for parsing the version on the base ISA vs other extensions (this could
perhaps be refactored).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147179/new/
https://reviews.llvm.org/D147179
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
