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