[PATCH] D95016: [Clang][RISCV] Add custom TableGen backend for riscv-vector intrinsics.

[Zakk Chen via Phabricator via llvm-commits]

khchen added inline comments.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:270
+      IsPointer(false), IsSize_t(false), IsPtrdiff_t(false),
+      ElementBitwidth(~0U), Scale(0) {
+  applyBasicType();
----------------
craig.topper wrote:
> Why is ElementBitwidth default ~0. Wouldn't 0 also be an invalid value?
I followed SVE did, but you are right, 0 is better.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:714
+  // Emit function arguments
+  if (CTypeOrder.size() > 1) {
+    OS << InputTypes[CTypeOrder[0]]->type_str() + " op0";
----------------
craig.topper wrote:
> Why is this > 1 and not >= 1 or !CTypeOrder.empty()?
Thanks, it's bug when I refactor code from output input vector to input only vector...


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:757
+  OS << "#error \"Vector intrinsics require the vector extension.\"\n";
+  OS << "#else\n\n";
+
----------------
craig.topper wrote:
> Can we just #endif here instead of the #else? If the error is emitted the preprocessor should stop and not process the rest of the file. Then we don't need to close it at the bottom of the file.
Good point. Thanks. I should think more when I copied code from SVE.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:873
+  // Dump switch body when the ir name changes from previous iteration.
+  RVVIntrinsic *PrevDef = Defs.begin()->get();
+  for (auto &Def : Defs) {
----------------
craig.topper wrote:
> Can we remember the PrevIRName StringRef instead?
I don't think so. We need `PrevDef` to emitCodeGenSwitchBody.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1027
+    return false;
+  bool NeedOR = false;
+  OS << "#if";
----------------
craig.topper wrote:
> I think you can use ListSeparator for this. It keeps track of the separator string and whether the first item has been printed. It's most often used with loops, but it should work here. I think there are many examples uses in llvm/utils/TableGen
thanks, it's more elegant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016