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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:267
+      D->Def = nullptr;
+      delete D;
+    }
----------------
Ayal wrote:
> 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.
Exactly. I added more details to the VPDef doc-comment. Is there another place that should have additional comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90558