[PATCH] D68577: [LV] Apply sink-after & interleave-groups as VPlan transformations (NFC)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7071
 
+  // ---------------------------------------------------------------------------
+  // Transform initial VPlan: Apply previously taken decisions, in order, to
----------------
gilr wrote:
> fhahn wrote:
> > Not sure how other feel, but I think it would be great if we could move this transform out of LoopVectorize.cpp , to group together VP2VP transforms. I think it would fit well into llvm/lib/Transforms/Vectorize/VPlanHCFGTransforms.h (although the name mentions HFCGTransforms, maybe it should be just VplanToVplanTransforms.h/cpp).
> > 
> > I could not spot anything that would prevent moving it to a different file on first glance.
> This is currently still ingredient-based, i.e. not a pure VPlan2VPlan transformation, and as you mention the VPlan2VPlan part it's basically just a moveAfter(). A VPlan-based sinkAfter() should not be based on ingredients (as stated in D46826) and instead might take a Recipe2Recipe map., but that seems a bit of an overkill for this patch. Modelling VPlan-based transformations is definitely worth a larger discussion.
Sounds good to me, let's not overcomplicate things.

Yep, D46826 is still not really suitable here yet, which I only realised after accidentally submitting the comment here. Sorry for the confusion.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7079
+    VPRecipeBase *Sink = RecipeBuilder.getRecipe(Entry.first);
+    Sink->removeFromParent();
+    Sink->insertAfter(RecipeBuilder.getRecipe(Entry.second));
----------------
gilr wrote:
> fhahn wrote:
> > This could just be  `Sink->moveAfter(RecipeBuilder.getRecipe(Entry.second)) `. I've added it in D46825 and now finally have a reason to commit it ;)
> Right. Will use it instead.
> Seems it doesn't update Parent, though. Will rewrite as a composition of the more basic removeFromParent(), insertAfter() to avoid duplicating that and the assertions.
Makes sense, thanks!

Originally I tried to mirror the implementation of Instruction::moveAfter, but it seems like it is only supposed to move instructions inside a basic block, although that's not really clear from the documentation.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:985
 
+  VPValue *getMask() {
+    // Mask is the last operand.
----------------
nit: it would be great to have a doc comment.


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

https://reviews.llvm.org/D68577