[PATCH] D100260: [VPlan] Merge predicated-triangle regions, after sinking.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:204
+      continue;
+    assert(!Region1->getCondBit() && !Region2->getCondBit());
+
----------------
Ayal wrote:
> Error message?
I moved the assert (with added message) to later and also updated `isPredicatedTriangle` to return the condition from the VPBranchOnMask recipe directly.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:232
+    for (VPRecipeBase &ToMove : make_early_inc_range(reverse(*Then1)))
+      ToMove.moveBefore(*Then2, Then2->begin());
+
----------------
Ayal wrote:
> moveBefore Then2->begin() should be fine, because Then2 has no phi's; worth a comment
Yes, but I think it is probably better to just use getFirstNonPhi(). Alternatively I can undo the change and add a comment.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:253
+
+      ToMove.moveBefore(*Merge2, Merge2->begin());
+    }
----------------
Ayal wrote:
> moveBefore Merge2->begin() is right, because we're moving phi's here; worth a comment.
Yes. I updated the variable name to `PhiToMove`, so it should be clearer. I did not add a comment because it seems a bit redundant to me after the renaming. But I'd be happy to add one if you think it is still valuable after the renaming.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:261
+    SmallVector<VPBlockBase *, 4> Preds(Region1->getPredecessors().begin(),
+                                        Region1->getPredecessors().end());
+    for (VPBlockBase *Pred : Preds) {
----------------
Ayal wrote:
> Alternatively (and more consistently) use make_early_inc_range?
done, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:194
+
+    // If a recipe is used by a first-order recurrence phi, we cannot move it.
+    if (any_of(*ThenRegion, [](VPRecipeBase &R) {
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > because ... it may conflict with sink-after?
> > > 
> > > comment here should probably state what it is we can move.
> > > Could there potentially be fusion-preventing dependencies between the two regions (packing/unpacking?), or does the lack of recipes in the 'middle' block guarantee their absence?
> > The problem with first-order recurrences is that the during code-gen the recurrence vector for the current iteration must be generated *before* all FOR users and *after* all parts of the recurrence value have been computed.
> > 
> > Note that this depends on D100260 and I added an additional test to cover the problematic case `recipe_in_merge_candidate_used_by_first_order_recurrence`.
> > The problem with first-order recurrences is that the during code-gen the recurrence vector for the current iteration must be generated *before* all FOR users and *after* all parts of the recurrence value have been computed.
> > 
> > Note that this depends on D100260 and I added an additional test to cover the problematic case `recipe_in_merge_candidate_used_by_first_order_recurrence`.
> 
> D100260 >> D104197, right?
Yes, D104197 is the correct one. I also addressed the latest comments there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260