[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:111
+  for (VPBasicBlock *VPBB :
+       make_early_inc_range(VPBlockUtils::blocksOnly<VPBasicBlock>(Iter))) {
     for (auto &Recipe : *VPBB) {
----------------
Why make_early_inc_range here, we're traversing all VPBB's only to populate WorkList with VPValues.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:153
+
+/// If \p R is a predicated triangle, return the 'then' block of the triangle.
+static std::pair<VPBasicBlock *, VPValue *>
----------------
Then perhaps "getPredicatedThenBlock(R)" would be a more accurate name than isPredicatedTriange()?

The first part checking if R's entry is a basic block with a single recipe being a Branch on Mask recipe, and returning its condition if so, can be done in a separate getPredicatedCondition(R)?

This would require later doing something like

```
  Cond1 = getPredicatedCondition(Region1);
  Cond2 = getPredicatedCondition(Region2);
  if (!Cond1 || Cond1 != Cond2)
    continue;
  Then1 = getPredicatedThenBlock(Region1);
  Then2 = getPredicatedThenBlock(Region2);
  if (!Then1 || !Then2)
    continue;

```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:199
+      continue;
+    auto *Succ = dyn_cast<VPBasicBlock>(Region1->getSingleSuccessor());
+    if (!Succ || !Succ->empty())
----------------
"Succ" >> "MiddleBasicBlock"?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:218
+    // the moment. The shuffled recurrence vector with all values needs to be
+    // generate before any users during code-gen currently.
+    auto IsImmovableRecipe = [](VPRecipeBase &R) {
----------------
Perhaps worth further clarifying: a recipe R feeding a first order recurrence phi must allow for a *vector* shuffle to be inserted immediately after it, and therefore if R is *scalarized and predicated* it must appear last in its basic block. In addition, other recipes may need to "sink after" R, so best if R not be moved at all.
A word on not moving multi-def'd recipes, e.g., they are not expected, and to simplify looking after all users?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:253
+
+      ToMove.moveBefore(*Merge2, Merge2->begin());
+    }
----------------
fhahn wrote:
> 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.
+1

Another option may be "Phi1", as it is a Phi of Merge1, of Region1. All Phi1's are moved to appear right before Phi2's. Or "Phi1ToMove".

Perhaps "IncV" >> "PredInst1", given that it is the recipe in Then1 feeding [PredInst]Phi1/Phi1ToMove.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:245
+    delete Region;
+  }
+
----------------
fhahn wrote:
> Ayal wrote:
> > delete DeletedBlocks?
> moved the deletion outside to clean up all blocks in DeletedBlocks.
Region1 itself should also be deleted, after its basic blocks are.
But suffice to do DeletedRegions.insert(Region1); i.e., deleting every Region1 takes care to destroy all its basic blocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260