[PATCH] D147179: [RISCV] Bump I, F, D, and A extension versions to 20191214 spec version

[Alex Bradbury via Phabricator via cfe-commits]

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