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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I realised I added a couple of response I didn't submit. Here they are



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:267
+      D->Def = nullptr;
+      delete D;
+    }
----------------
Ayal wrote:
> Better avoid deleting all VPValues here - at-least those who are VPInstructions/single-valued-recipes?
I think conceptually it would make sense that deleting a VPDef also deletes all values defined by it? 

The single value def case should work out I think, because the VPVAlue destructor should be called before the VPDef one, and the VPValue destructor removes the value from DefinedValues.

If you think it's too subtle though we can delegate this to the concrete subclasses that define multiple values.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:93
   virtual ~VPValue() {
     assert(Users.empty() && "trying to delete a VPValue with remaining users");
   }
----------------
Ayal wrote:
> detach from Def?
Done, now the destructor removes the VPValue from its def.


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

I'd prefer to change to protected once we need to?

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

I've added `removeDefinedValue`


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:524
 
+struct VPMultiDef : public VPDef {
+  SmallVector<VPValue *, 4> Defs;
----------------
Ayal wrote:
> VPMultiValuedDef or VPDoubleValuedDef? It's a single Def, with multiple Values.
The naming is still from earlier versions. Changed to `VPDoubleValueDef`.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:525
+struct VPMultiDef : public VPDef {
+  SmallVector<VPValue *, 4> Defs;
+
----------------
Ayal wrote:
> Unused; suffice to hold the VPValues by VPDef alone.
Yep, left over from an earlier version. Removed now.


================
Comment at: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp:529
+    new VPValue(nullptr, this);
+    new VPValue(nullptr, this);
+  }
----------------
Ayal wrote:
> 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...
I updated the VPDef destructor to delete all defined values. So no delete's should be needed here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90558