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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:249
+
+  /// Add \p V as a defined value by the VPDef.
+  void removeDefinedValue(VPValue *V) {
----------------
`/// Add >> /// Remove`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:267
+      D->Def = nullptr;
+      delete D;
+    }
----------------
fhahn wrote:
> 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.
Ahh, ok, D90565 changes the multiple inheritance order of VPInstruction, to first inherit from VPRecipeBase and last from VPValue, so indeed the latter's destructor (which now removes itself from its VPDef) should be called before the former. Worth a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90558