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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:116
+  D = 1 << 2,
+  ZFH = 1 << 3
+};
----------------
Zfh


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:241
+  case 8:
+    ExpResult = Log2LMUL + 3;
+    break;
----------------
Please be consistent and use Log2 rather than Exp


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:259-260
+
+LMULType &LMULType::operator*=(unsigned RHS) {
+  this->Log2LMUL = this->Log2LMUL + RHS;
+  return *this;
----------------
That's not how multiplication works. This is exponentiation. Multiplication would be `Log2LMul + log2(RHS)`. Please don't abuse operators like this.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:325
+  std::string &S = BuiltinStr;
+  if (IsVoid) {
+    S = "v";
----------------
This really needs to be an enum not a bunch of mutually-exclusive booleans, which I though I suggested in the past?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:450-451
+      S += "uint";
+    // Vector bool is special case, the formulate is `vbool<N>_t =
+    // MVT::nxv<64/N>i1` ex. vbool16_t = MVT::
+    if (IsBool && isVector())
----------------
Please try and avoid wrapping code across lines


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:624
+  }
+  // Init RISCV_Extensions
+  for (const auto &T : OutInTypes) {
----------------
Blank line


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:685
+  OS << "};\n";
+  OS << "break;\n";
+}
----------------
This is missing indentation?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:870
+    PrevDef = Def.get();
+    OS << "case RISCV::BI__builtin_rvv_" << Def->getName() << ":\n";
+  }
----------------
Needs indentation?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:567
+  // Compute type transformers
+  for (char I : Transformer.take_front(Transformer.size() - 1)) {
+    switch (I) {
----------------
craig.topper wrote:
> Can we do Transformer = Transformer.drop_back() right before this loop. That take_front code is harder to think about.
Or would it be better as a pop_back in the switch above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016