[PATCH] D100260: [VPlan] Merge predicated-triangle regions, after sinking.
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jun 27 07:29:16 PDT 2021
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
More information about the llvm-commits
mailing list