[PATCH] D108694: [RISCV] Add the zvl extension according to the v1.0 spec

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:774
+  while (!WorkList.empty()) {
+    auto ExtName = WorkList.pop_back_val();
+    auto I = llvm::lower_bound(ImpliedExts, ExtName);
----------------
Use StringRef instead of auto here.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:775
+    auto ExtName = WorkList.pop_back_val();
+    auto I = llvm::lower_bound(ImpliedExts, ExtName);
+    if (I != std::end(ImpliedExts) && I->Name == ExtName) {
----------------
You can keep this auto though. iterators are ugly.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:188
+                       [FeatureStdExtZvl32768b]>;
+def HasStdExtZvl : Predicate<"Subtarget->hasStdExtZvl()">;
+
----------------
Is this used?


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:123
+  if (RVVVectorBitsMax < ZvlLen)
+    report_fatal_error(Twine("riscv-v-vector-bits-max specified is lower "
+                             "than the Zvl*b limitation"));
----------------
Why do we need an explicit Twine construction here?


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:142
+  if (RVVVectorBitsMin != 0 && RVVVectorBitsMin < ZvlLen)
+    report_fatal_error(Twine("riscv-v-vector-bits-min specified is lower "
+                             "than the Zvl*b limitation"));
----------------
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108694