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

Sun Oct 13 03:50:34 PDT 2019

gilr marked 2 inline comments as done.
gilr added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7071

+  // ---------------------------------------------------------------------------
+  // Transform initial VPlan: Apply previously taken decisions, in order, to
----------------
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.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7079
+    VPRecipeBase *Sink = RecipeBuilder.getRecipe(Entry.first);
+    Sink->removeFromParent();
+    Sink->insertAfter(RecipeBuilder.getRecipe(Entry.second));
----------------
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.

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

https://reviews.llvm.org/D68577