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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

This is a very incomplete review, but I need to go eat dinner



================
Comment at: clang/include/clang/Basic/riscv_vector.td:201
+// gen-riscv-vector-test.
+// gen-riscv-v-tests.sh will define each marco to generate each intrinsic test
+// in different files. It mean adding the new definition also need to update
----------------
marco->macro


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:124
+
+// TODO rafactor Intrinsic class design after support all intrinsic combination.
+class Intrinsic {
----------------
rafactor->refactor


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1000
+  SmallVector<std::string, 8> ProtoSeq;
+  const StringRef Pirmaries("evwqom0ztc");
+  size_t start = 0;
----------------
Is this supposed to be Primaries or some other word?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1055
+  RVVTypes Types;
+  for (std::string Proto : PrototypeSeq) {
+    auto T = computeType(BT, LMUL, Proto);
----------------
Can Proto be a const std::string & ?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1057
+    auto T = computeType(BT, LMUL, Proto);
+    if (!T.hasValue()) {
+      return llvm::None;
----------------
Drop curly braces


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1068
+                                             StringRef Proto) {
+  TypeString Idx = Twine(BT + utostr(LMUL) + Proto).str();
+  // search first
----------------
Use Twine(LMUL)  instead of utostr. That should avoid creating std::string


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1112
+    }
+    if (ExtStrings.size())
+      OS << "#endif\n\n";
----------------
!ExtStrings.empty()


================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:21
+gen_tests(){
+# op_list have marco name used in riscv_vector.td
+  local op_list="VADD VFADD"
----------------
marco->macro


================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:22
+# op_list have marco name used in riscv_vector.td
+  local op_list="VADD VFADD"
+  local option="$1"
----------------
It feels a little weird that this list is in the script and not derived from the td file automatically somehow. Ideally we wouldn't have to update the script every time a new set of intrinsics is added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016