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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:159
+  ReversePostOrderTraversal<VPBlockRecursiveTraversalWrapper<VPBlockBase *>>
+      RPOT(VPBlockRecursiveTraversalWrapper<VPBlockBase *>(Plan.getEntry()));
+
----------------
Ayal wrote:
> Is RPOT order vital, or would a simple DFS do, given that the pairs of regions handled are dom-postdom pairs?
No that's not needed. Updated to collect the relevant blocks using `VPBlockUtils::blocksOnly<VPRegionBlock>`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:180
+    auto *SuccRegion = dyn_cast<VPRegionBlock>(Succ->getSingleSuccessor());
+    if (!SuccRegion || Region->getPredicate() != SuccRegion->getPredicate())
+      continue;
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:197
+            auto *PhiR = dyn_cast<VPWidenPHIRecipe>(UI);
+            if (!PhiR || PhiR->getRecurrenceDescriptor())
+              continue;
----------------
Ayal wrote:
> Ayal wrote:
> > May be good to name this lambda, e.g., IsImmovableRecipe.
> > 
> > Check first-order recurrence (more) directly, by
> > 
> > ```
> >   if (PhiR && !PhiR->getRecurrenceDescriptor())
> >     return true
> > ```
> > ?
> > Would be good to have a clear API for such a query, if not a dedicated isa<> recipe.
> > 
> ThenRegion is expected to have no phi recipes, let alone header phi recipes, but only replicate recipes? Perhaps something potentially worth Verifying.
> 
> May be clearer to rename variables Region1/ThenBB1/MergeBB1, Region2/ThenBB2/MergeBB2, MiddleBB
> ThenRegion is expected to have no phi recipes, let alone header phi recipes, but only replicate recipes? Perhaps something potentially worth Verifying.

Added the assertion.

> May be clearer to rename variables Region1/ThenBB1/MergeBB1, Region2/ThenBB2/MergeBB2, MiddleBB

Great idea, the suggested names are much clearer!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:215
+    // merge block. Update all users inside the successor region to use the
+    // original values.
+    for (VPRecipeBase &ToMove : make_early_inc_range(reverse(*MergeRegion))) {
----------------
Ayal wrote:
> Note: this takes care of fusion-admitting, lane-by-lane dependencies, right?
Yes, it updates the uses outside the merged region.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:216
+    // original values.
+    for (VPRecipeBase &ToMove : make_early_inc_range(reverse(*MergeRegion))) {
+      VPValue *IncV = cast<VPPredInstPHIRecipe>(&ToMove)->getOperand(0);
----------------
Ayal wrote:
> Is the reverse required? Could repeatedly take (r)begin until empty.
> 
> Initially each MergeRegion has a single PredInstPHIRecipe, but multiple recipes would appear after merging, right? Worth testing merging three regions into one, if not done so already.
I added a test `merge_3_replicate_region`, which exposed a problem with multiple `VPPredInstPHIRecipes` in the same block. I put up D104188 to fix that.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:225
+            continue;
+          U->setOperand(I, IncV);
+        }
----------------
Ayal wrote:
> `I` could appear as multiple operands of `U`, so avoid early-exit on first match, right? Perhaps worth a test.
> 
> Assert that at-least one such operand was found and (re)set?
Yes, there could be multiple uses of  `ToMove` in `U`. I still need to add a test case for that.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:245
+    delete Region;
+  }
+
----------------
Ayal wrote:
> delete DeletedBlocks?
moved the deletion outside to clean up all blocks in DeletedBlocks.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:153
+  auto *ThenBB = dyn_cast<VPBasicBlock>(EntryBB->getSuccessors()[0]);
+  auto *MergeBB = dyn_cast<VPBasicBlock>(EntryBB->getSuccessors()[1]);
+  if (!ThenBB || !MergeBB)
----------------
Ayal wrote:
> rely on order of successors?
Yes. I updated the code to check if the single successor of `ThenBB` is `MergeBB`. If that's not the case, they are swapped and must pass the check below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260