[PATCH] D90558: [VPlan] Add VPDef class.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Looks good to me, adding minor comments, thanks for pushing this forward!

In the summary:

> If we have a VPDef, we have to traverse all operands of the VPDef.

//If we have a VPDef, it is defined inside the region by a recipe, which is a VPUser, and the upwards def-use chain traversal continues by traversing all its operands.//



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:36
 class VPRecipeBase;
+class VPDef;
 
----------------
Try to keep in lex order?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:49
   friend class VPRecipeBase;
+  friend class VPDef;
 
----------------
ditto...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:93
   virtual ~VPValue() {
     assert(Users.empty() && "trying to delete a VPValue with remaining users");
   }
----------------
detach from Def?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:223
+
+/// This class augment a recipe with a set of VPValues defined by the recipe. It
+/// allows recipes to define zero, one or multiple VPValues.
----------------
augment[s]


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:231
+
+  /// Add \p V as a defined value by the VPDef.
+  void addDefinedValue(VPValue *V) {
----------------
Worth having 'protected' rather than 'private', to allow recipes access?

May also need the ability to remove/replace a defined value.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:260
+
+  /// Returns an ArrayRef of the defined values by the VPDef.
+  ArrayRef<VPValue *> defined_values() { return DefinedValues; }
----------------
'defined values' >> 'values defined'


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:261
+  /// Returns an ArrayRef of the defined values by the VPDef.
+  ArrayRef<VPValue *> defined_values() { return DefinedValues; }
+
----------------
Agree with tidy: defined_values() >> definedValues() or getDefinedValues().


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:524
 
+struct VPMultiDef : public VPDef {
+  SmallVector<VPValue *, 4> Defs;
----------------
VPMultiValuedDef or VPDoubleValuedDef? It's a single Def, with multiple Values.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:525
+struct VPMultiDef : public VPDef {
+  SmallVector<VPValue *, 4> Defs;
+
----------------
Unused; suffice to hold the VPValues by VPDef alone.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:529
+    new VPValue(nullptr, this);
+    new VPValue(nullptr, this);
+  }
----------------
Add delete's, for completeness? Adding them inside ~VPMultiDef() will kill the VPValues before ~VPDef() checks they have no users and checks/resets their Def's... have ~VPValue() take itself out of its VPDef, as commented above?
Or delete the VPValues after deleting the VPMultiDef...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90558