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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This looks good to me, adding final minor nits.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:204
+      continue;
+    assert(!Region1->getCondBit() && !Region2->getCondBit());
+
----------------
Error message?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:232
+    for (VPRecipeBase &ToMove : make_early_inc_range(reverse(*Then1)))
+      ToMove.moveBefore(*Then2, Then2->begin());
+
----------------
moveBefore Then2->begin() should be fine, because Then2 has no phi's; worth a comment


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:253
+
+      ToMove.moveBefore(*Merge2, Merge2->begin());
+    }
----------------
moveBefore Merge2->begin() is right, because we're moving phi's here; worth a comment.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:180
+    auto *SuccRegion = dyn_cast<VPRegionBlock>(Succ->getSingleSuccessor());
+    if (!SuccRegion || Region->getPredicate() != SuccRegion->getPredicate())
+      continue;
----------------
fhahn wrote:
> Ayal wrote:
> > Ayal wrote:
> > > This may be somewhat misleading, and currently untestable: replicated regions are (currently) formed for unpredicatable instructions, by 'legacy' vectorizer which does not model block predicates at all. Assert the predicates are null?
> > OTOH, need to check that both VPBranchOnMaskRecipes use the same mask. Potentially have getPredicate() retrieve this mask? Test that two regions having distinct masks are not merged.
> `getPredicate` was wrong. Currently `CondBit` is kept up-to-date. I updated the code to use it. Perhaps we should remove the CondBit field and implement `getCondBit` with a lookup of `branch-on-mask` recipes as follow-up?
> `getPredicate` was wrong. Currently `CondBit` is kept up-to-date. I updated the code to use it. Perhaps we should remove the CondBit field and implement `getCondBit` with a lookup of `branch-on-mask` recipes as follow-up?

+1!


================
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) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260