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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks!
Adding some minor optional nits.
May be worth noting why no fusion-preventing dependencies are expected.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:153
+/// If \p R is a region with a VPBranchOnMaskRecipe in the entry block, return
+/// the condition.
+VPValue *getPredicatedCondition(VPRegionBlock *R) {
----------------
"the condition" >> "the mask" ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:173
+    return nullptr;
+
+  if (ThenBB->getSingleSuccessor() != MergeBB)
----------------
Could alternatively do

```
  if (Succ0->getNumSuccessors() + Succ1->getNumSuccessors() != 1)
    return nullptr;
  if (Succ0->getSingleSuccessor() == Succ1)
    return Succ0;
  if (Succ1->getSingleSuccessor() == Succ0)
    return Succ1;
  return nullptr;
```


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:184
+bool VPlanTransforms::mergeReplicateRegions(VPlan &Plan) {
+  SetVector<VPBlockBase *> DeletedBlocks;
+  bool Changed = false;
----------------
`SetVector<VPRegionBlock *> DeletedRegions` ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:204
+    auto *MiddleBasicBlock =
+        dyn_cast<VPBasicBlock>(Region1->getSingleSuccessor());
+    if (!MiddleBasicBlock || !MiddleBasicBlock->empty())
----------------
could use `dyn_cast_or_null<VPBasicBlock>(Region1->getSingleSuccessor());` here instead of filtering regions with non single successors earlier (which probably do not exist).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:258
+      VPValue *PredInst1 =
+          cast<VPPredInstPHIRecipe>(&Phi1ToMove)->getOperand(0);
+      for (VPUser *U : Phi1ToMove.getVPValue()->users()) {
----------------
worth introducing `VPPredInstPHIRecipe::getPredInst() { return getOperand(0); }` ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:274
+    // Finally, remove the first region.
+    DeletedBlocks.insert(Region1);
+    for (VPBlockBase *Pred : make_early_inc_range(Region1->getPredecessors())) {
----------------
Perhaps looks somewhat better if Region1 is inserted into DeletedBlocks (DeletedRegions) after disconnecting it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100260