[PATCH] D84679: [VPlan] Disconnect VPValue and VPUser.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This looks good to me, as part of the effort to support VPlan def-use modeling and traversals.
Note that top-down traversal from VPValue to its VPUsers, will now need to check if each VPUser isa single-def recipe/VPInstruction in order to continue downwards to its VPUsers, etc., facilitating multi-def recipes.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanSLP.cpp:164
   for (VPValue *V : Values) {
-    auto *U = cast<VPUser>(V);
+    auto *U = cast<VPInstruction>(V);
     Operands.push_back(U->getOperand(OperandIndex));
----------------
nit: maybe worth reiterating next to the cast that
```
// Currently we only support VPInstructions.
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanSLP.cpp:224
 static unsigned getLAScore(VPValue *V1, VPValue *V2, unsigned MaxLevel,
                            VPInterleavedAccessInfo &IAI) {
   if (!isa<VPInstruction>(V1) || !isa<VPInstruction>(V2))
----------------
nit: worth simplifying by setting here:

```
auto I1 = dyn_cast<VPInstruction>(V1);
auto I2 = dyn_cast<VPInstruction>(V2);
```
and reuse them below instead of isa<> and cast<>'s?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:13
 /// VPlan models the following entities:
 /// VPValue
 ///  |-- VPUser
----------------
The documentation of this class hierarchy deserves an update, as does docs/Proposals/VectorizationPlan.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84679