[PATCH] D112408: [WIP][RISCV] Add the zve extension according to the v1.0-rc2 spec

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added a comment.

I think the rest of my comments would be to do with `zvl` so I'll leave it there to avoid repetition.



================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:65
+  unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxEew() const { return MaxEew; }
+  unsigned getMaxEewFp() const { return MaxEewFp; }
----------------
Aside from the discussion about EEW vs. ELEN, something about the capitalization irks me. I realise we already have `XLen` but `Eew` looks... wrong. If other people disagree then that's fine.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:67
     {"zvlsseg", RISCVExtensionVersion{0, 10}},
+    {"zvl32b", RISCVExtensionVersion{0, 10}},
+    {"zvl64b", RISCVExtensionVersion{0, 10}},
----------------
Should this be in this patch? Or has some rebasing gone wrong and introduced code for D108694?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:733
+
+      {"zvl64b", {"zvl32b"}},
+      {"zvl128b", {"zvl64b"}},
----------------
Again, should `zvl` code be in this patch?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:765
+    if (Implication != Implications.end()) {
+      for (auto ImpliedExtName : Implication->second) {
+        if (WorkList.count(ImpliedExtName))
----------------
Really minor, but here you're using `auto` for `StringRef` but earlier and elsewhere it's `auto &`. I'm not sure which is preferred. Presumably `StringRef`s are cheap to copy and `auto` is fine? If `auto &` is more prominent in this file then go with that.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:787
 
+void RISCVISAInfo::updateMinVLen() {
+  for (auto &Ext : Exts) {
----------------
`zvl` patch?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:468
+
+  if (Error Result = ISAInfo->checkDependency())
+    return std::move(Result);
----------------
I'm not the most familiar with this API, but do we really need to `checkDependency` here when it's done in the next line?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:696
+
+  if (Error Result = ISAInfo->checkDependency())
+    return std::move(Result);
----------------
Same here. Duplicate `checkDependency`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112408/new/

https://reviews.llvm.org/D112408