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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:69
+    Def->removeDefinedValue(this);
+  assert(!Def && "trying to delete a VPValue with a linked VPDef");
+}
----------------
nit: this assert that removeDefinedValue(V) sets V->Def to null seems a bit redundant; its comment describes what is done if (Def) ...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:267
+      D->Def = nullptr;
+      delete D;
+    }
----------------
fhahn wrote:
> 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?
Very good, thanks.
Probably worth noting also in D90565 when setting the (new) multiple inheritance order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90558