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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
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:
> 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.


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


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260