-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[VPlan] Simplify branch on False in VPlan transform (NFC). #140409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
063d70f
f5ee27b
74eddb5
61779cf
801b98d
069767b
d3df2c3
3f3f002
69ad1de
9233672
6ced27d
2b76566
5797781
761d4ca
ff89acf
fa0eab1
57652c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -887,11 +887,6 @@ class VPInstruction : public VPRecipeWithIRFlags, | |||||||||
| SLPStore, | ||||||||||
| ActiveLaneMask, | ||||||||||
| ExplicitVectorLength, | ||||||||||
| /// Creates a scalar phi in a leaf VPBB with a single predecessor in VPlan. | ||||||||||
| /// The first operand is the incoming value from the predecessor in VPlan, | ||||||||||
| /// the second operand is the incoming value for all other predecessors | ||||||||||
| /// (which are currently not modeled in VPlan). | ||||||||||
| ResumePhi, | ||||||||||
| CalculateTripCountMinusVF, | ||||||||||
| // Increment the canonical IV separately for each unrolled part. | ||||||||||
| CanonicalIVIncrementForPart, | ||||||||||
|
|
@@ -1127,6 +1122,8 @@ class VPPhiAccessors { | |||||||||
| return getAsRecipe()->getNumOperands(); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| void removeIncomingValue(VPBlockBase *VPB) const; | ||||||||||
|
||||||||||
| void removeIncomingValue(VPBlockBase *VPB) const; | |
| /// Removes the incoming value corresponding to \p IncomingBlock, which must be a predecessor. | |
| void removeIncomingValueFor(VPBlockBase *IncomingBlock) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done thanks
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert(R->getNumOperands() == Parent->getNumPredecessors()); | |
| assert(R->getNumOperands() == Parent->getNumPredecessors() && "Number of phi operands must match number of predecessors"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done thanks
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: may be clearer to use distance as in
| auto I = find(Parent->getPredecessors(), VPB); | |
| auto &Preds = R->getParent()->getPredecessors(); | |
| assert(R->getNumOperands() == Preds.size() && "Number of phi operands must match number of predecessors"); | |
| unsigned Position = std::distance(Preds.begin(), find(Preds, IncomingBlock)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also moved to VPlanRecipes.cpp
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -501,8 +501,12 @@ void VPlanTransforms::prepareForVectorization( | |||||
| cast<VPBasicBlock>(HeaderVPB), | ||||||
| cast<VPBasicBlock>(LatchVPB), Range); | ||||||
| HandledUncountableEarlyExit = true; | ||||||
| } else { | ||||||
| for (VPRecipeBase &R : cast<VPIRBasicBlock>(EB)->phis()) { | ||||||
|
||||||
| if (auto *PhiR = dyn_cast<VPIRPhi>(&R)) | ||||||
|
||||||
| PhiR->removeIncomingValue(Pred); | ||||||
|
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| cast<VPBasicBlock>(Pred)->getTerminator()->eraseFromParent(); | ||||||
| VPBlockUtils::disconnectBlocks(Pred, EB); | ||||||
| } | ||||||
|
|
@@ -535,45 +539,33 @@ void VPlanTransforms::prepareForVectorization( | |||||
| // Thus if tail is to be folded, we know we don't need to run the | ||||||
| // remainder and we can set the condition to true. | ||||||
| // 3) Otherwise, construct a runtime check. | ||||||
|
|
||||||
| if (!RequiresScalarEpilogueCheck) { | ||||||
| if (auto *LatchExitVPB = MiddleVPBB->getSingleSuccessor()) | ||||||
| VPBlockUtils::disconnectBlocks(MiddleVPBB, LatchExitVPB); | ||||||
| VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH); | ||||||
| VPBlockUtils::connectBlocks(Plan.getEntry(), ScalarPH); | ||||||
| Plan.getEntry()->swapSuccessors(); | ||||||
|
|
||||||
| // The exit blocks are unreachable, remove their recipes to make sure no | ||||||
| // users remain that may pessimize transforms. | ||||||
| for (auto *EB : Plan.getExitBlocks()) { | ||||||
| for (VPRecipeBase &R : make_early_inc_range(*EB)) | ||||||
| R.eraseFromParent(); | ||||||
| } | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| // The connection order corresponds to the operands of the conditional branch, | ||||||
| // with the middle block already connected to the exit block. | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Above two lines should be discarded? Or updated to end with "if needed"?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped by accident, restored, thanks |
||||||
| VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH); | ||||||
| // Also connect the entry block to the scalar preheader. | ||||||
| // TODO: Also introduce a branch recipe together with the minimum trip count | ||||||
| // check. | ||||||
| VPBlockUtils::connectBlocks(Plan.getEntry(), ScalarPH); | ||||||
| Plan.getEntry()->swapSuccessors(); | ||||||
|
|
||||||
| if (MiddleVPBB->getNumSuccessors() != 2) | ||||||
|
||||||
| return; | ||||||
|
|
||||||
| auto *ScalarLatchTerm = TheLoop->getLoopLatch()->getTerminator(); | ||||||
| // Here we use the same DebugLoc as the scalar loop latch terminator instead | ||||||
| // of the corresponding compare because they may have ended up with | ||||||
| // different line numbers and we want to avoid awkward line stepping while | ||||||
| // debugging. Eg. if the compare has got a line number inside the loop. | ||||||
|
||||||
| // debugging. Eg. if the compare has got a line number inside the loop. | |
| // debugging. E.g., if the compare has got a line number inside the loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated thanks
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1847,11 +1847,21 @@ static void removeBranchOnCondTrue(VPlan &Plan) { | |
| using namespace llvm::VPlanPatternMatch; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated, thanks |
||
| for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>( | ||
| vp_depth_first_shallow(Plan.getEntry()))) { | ||
| VPValue *Cond; | ||
| if (VPBB->getNumSuccessors() != 2 || VPBB == Plan.getEntry() || | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Independent: why is the entry block excluded?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Entry will have 2 successors w/o branch. Will be fixed by introducing a branch recipe, possibly on an opaque condition, when connecting entry to vector.ph/scalar.ph.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right, perhaps worth a clarifying TODO. |
||
| !match(&VPBB->back(), m_BranchOnCond(m_True()))) | ||
| !match(&VPBB->back(), m_BranchOnCond(m_VPValue(Cond)))) | ||
| continue; | ||
|
|
||
| VPBasicBlock *RemovedSucc = cast<VPBasicBlock>(VPBB->getSuccessors()[1]); | ||
| unsigned RemovedIdx; | ||
| if (match(Cond, m_True())) | ||
| RemovedIdx = 1; | ||
| else if (match(Cond, m_False())) | ||
| RemovedIdx = 0; | ||
| else | ||
| continue; | ||
|
|
||
| VPBasicBlock *RemovedSucc = | ||
| cast<VPBasicBlock>(VPBB->getSuccessors()[RemovedIdx]); | ||
| const auto &Preds = RemovedSucc->getPredecessors(); | ||
| assert(count(Preds, VPBB) == 1 && | ||
| "There must be a single edge between VPBB and its successor"); | ||
|
|
@@ -1860,12 +1870,14 @@ static void removeBranchOnCondTrue(VPlan &Plan) { | |
| // Values coming from VPBB into ResumePhi recipes of RemoveSucc are removed | ||
| // from these recipes. | ||
| for (VPRecipeBase &R : make_early_inc_range(*RemovedSucc)) { | ||
|
||
| assert((!isa<VPIRInstruction>(&R) || | ||
| !isa<PHINode>(cast<VPIRInstruction>(&R)->getInstruction())) && | ||
| !isa<VPHeaderPHIRecipe>(&R) && | ||
| "Cannot update VPIRInstructions wrapping phis or header phis yet"); | ||
| auto *VPI = dyn_cast<VPInstruction>(&R); | ||
| if (!VPI || VPI->getOpcode() != VPInstruction::ResumePhi) | ||
| if (isa<VPIRPhi>(&R)) { | ||
| assert(RemovedSucc->getNumPredecessors() == 1); | ||
|
||
| cast<VPIRPhi>(&R)->removeIncomingValue(VPBB); | ||
| continue; | ||
| } | ||
|
|
||
| auto *VPI = dyn_cast<VPPhi>(&R); | ||
| if (!VPI) | ||
| break; | ||
| VPBuilder B(VPI); | ||
| SmallVector<VPValue *> NewOperands; | ||
|
|
@@ -1875,9 +1887,8 @@ static void removeBranchOnCondTrue(VPlan &Plan) { | |
| continue; | ||
| NewOperands.push_back(Op); | ||
| } | ||
| VPI->replaceAllUsesWith(B.createNaryOp(VPInstruction::ResumePhi, | ||
| NewOperands, VPI->getDebugLoc(), | ||
| VPI->getName())); | ||
| VPI->replaceAllUsesWith( | ||
| B.createScalarPhi(NewOperands, VPI->getDebugLoc(), VPI->getName())); | ||
|
||
| VPI->eraseFromParent(); | ||
| } | ||
| // Disconnect blocks and remove the terminator. RemovedSucc will be deleted | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: take the number of ScalarPH predecessors once, here, reuse it repeatedly inside the loop, asserting it is one more than the number of ResumePhi's operands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks