[PATCH] D91441: [VP] Build VP SDNodes

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:33
+// Register a new VP SDNode and begin its property scope.
+// When the SDNode scope is nested within a VP intrinsic scope, it is implicitly registered as the cannonical SDNode for this VP intrinsic.
+// There is one VP intrinsic that maps directly to one SDNode that goes by the
----------------
typo: `canonical`


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:42
+// \p TDNAME   The name of the TableGen definition of this SDNode.
+//             name of the intrinsic and the TableGen def of the SDNode).
+// \p MASKPOS  The mask operand position.
----------------
Missing `(` somewhere?


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:44
+// \p MASKPOS  The mask operand position.
+// \p EVLPOS   The explicit vector length operand position.
+#ifndef BEGIN_REGISTER_VP_SDNODE
----------------
Mismatch between `EVLPOS` and `VLENPOS`


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:72
+
+#define END_REGISTER_VP(INTRIN, SDID) \
+END_REGISTER_VP_INTRINSIC(INTRIN) \
----------------
I'm thinking of these as scopes as the documentation describes them, and it strikes me that the "INTRINSIC" and "SDNODE" scopes overlap here when using `BEGIN_REGISTER_VP` and `END_REGISTER_VP`:

```
BEGIN_REGISTER_VP_INTRINSIC
  BEGIN_REGISTER_VP_SDNODE
END_REGISTER_VP_INTRINSIC
  END_REGISTER_VP_SDNODE
```

Is there a reason for this, or should it be balanced?

```
BEGIN_REGISTER_VP_INTRINSIC
  BEGIN_REGISTER_VP_SDNODE
  END_REGISTER_VP_SDNODE
END_REGISTER_VP_INTRINSIC
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:762
   void visitConstrainedFPIntrinsic(const ConstrainedFPIntrinsic &FPI);
+  void visitVectorPredicationIntrinsic(const VPIntrinsic &FPI);
 
----------------
parameter named `VPIntrin` like in the function def?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91441