[PATCH] D91441: [VP] Build VP SDNodes

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:24
+#ifndef BEGIN_REGISTER_VP_INTRINSIC
+#define BEGIN_REGISTER_VP_INTRINSIC(VPID, MASKPOS, VLENPOS)
 #endif
----------------
Should this use EVLPOS instead of VLENPOS?


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:37
+// scopes for both the VPIntrinsic and the SDNode at once.
+// \p SDID     The SelectionDAG Node id (eg VP_ADD).
+// \p LEGALPOS The operand position of the SDNode that is used for legalizing
----------------
I think ISDOPC or something might be better than SDID. I don't think we tend to call them ids for SelectionDAG. In SelectionDAG terms, id would tend to mean the NodeID field in SDNode.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:59
+//
+// \p INTRIN   The cannonical name (eg `vp_add`, which at the same time is the
+//             name of the intrinsic and the TableGen def of the SDNode).
----------------
canonical*


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7030
 
+static Optional<unsigned> getISDForVPIntrinsic(const VPIntrinsic &VPIntrin) {
+  Optional<unsigned> ResID;
----------------
This ever expected to return an Optional with no value set? It looks like the only caller just calls getValue(). Should we just have an llvm_unreachable in the switch instead?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7055
+  SmallVector<SDValue, 7> OpValues;
+  for (int i = 0; i < (int)VPI.getNumArgOperands(); ++i) {
+    OpValues.push_back(getValue(VPI.getArgOperand(i)));
----------------
Drop curlies.


================
Comment at: llvm/test/CodeGen/VE/Vector/vp_add.ll:1
+; RUN: (llc %s -march=ve -mattr=+vpu -o /dev/null || true) |& FileCheck %s
+
----------------
Can we use "not llc" instead of "|| true"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91441