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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Nice optimization exercising VPlan capabilities!
Should always be a win, so ok to avoid cost-model considerations, although cost improvement is left unaccounted for.
An alternative may be to try and build larger replicating regions to begin with, but sinkScalarOperands may contribute as well.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:152
+  if (MergeBB->getNumSuccessors() != 0 || ThenBB->getNumSuccessors() != 1 ||
+      ThenBB->getSuccessors()[0] != MergeBB)
+    return nullptr;
----------------
Can check `|| ThenBB->getSingleSuccessor() != MergeBB`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:159
+  ReversePostOrderTraversal<VPBlockRecursiveTraversalWrapper<VPBlockBase *>>
+      RPOT(VPBlockRecursiveTraversalWrapper<VPBlockBase *>(Plan.getEntry()));
+
----------------
Is RPOT order vital, or would a simple DFS do, given that the pairs of regions handled are dom-postdom pairs?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:175
+
+    auto *Succ = dyn_cast<VPBasicBlock>(Region->getSingleSuccessor());
+    if (!Succ || !Succ->getSingleSuccessor() || !Succ->empty())
----------------
dyn_cast_or_null would also take care of checking if Region has a single successor?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:179
+
+    auto *SuccRegion = dyn_cast<VPRegionBlock>(Succ->getSingleSuccessor());
+    if (!SuccRegion || Region->getPredicate() != SuccRegion->getPredicate())
----------------
dyn_cast_or_null would also take care of checking if Succ has a single successor?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:197
+            auto *PhiR = dyn_cast<VPWidenPHIRecipe>(UI);
+            if (!PhiR || PhiR->getRecurrenceDescriptor())
+              continue;
----------------
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.



================
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))) {
----------------
Note: this takes care of fusion-admitting, lane-by-lane dependencies, right?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:225
+            continue;
+          U->setOperand(I, IncV);
+        }
----------------
`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?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:245
+    delete Region;
+  }
+
----------------
delete 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)
----------------
rely on order of successors?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260