-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[VPlan] Remove loop region in optimizeForVFAndUF. #108378
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 16 commits
d3614bc
5f8fabe
c68ddd1
f0421c6
4a0eb12
9499aaa
a4843b5
f5d2bc6
1222e23
706b681
71436fc
38cbdf6
cc43362
e758945
d4e8c7e
1d4b2e6
98529f3
0c76e9d
dd45cad
e72a71f
407dbc1
af48fcc
f51412a
f2b5e53
1f4febc
3d412a1
56a4181
88fcf60
303ce93
3f016cb
430c369
f9db2d0
cabc591
057f2e9
60a046a
fe2c3a5
26c94b1
92c0ccc
05b2e4e
df67f2e
e17003f
f180edb
71ff80a
911c50a
4f74827
d17571d
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2471,6 +2471,8 @@ InnerLoopVectorizer::getOrCreateVectorTripCount(BasicBlock *InsertBlock) { | |||||
| /// scalar preheader. | ||||||
| static void introduceCheckBlockInVPlan(VPlan &Plan, BasicBlock *CheckIRBB) { | ||||||
| VPBlockBase *ScalarPH = Plan.getScalarPreheader(); | ||||||
| // FIXME: Cannot get the vector preheader at the moment if the vector loop | ||||||
| // region has been removed. | ||||||
| VPBlockBase *VectorPH = Plan.getVectorPreheader(); | ||||||
| VPBlockBase *PreVectorPH = VectorPH->getSinglePredecessor(); | ||||||
| if (PreVectorPH->getNumSuccessors() != 1) { | ||||||
|
|
@@ -3066,6 +3068,9 @@ void InnerLoopVectorizer::fixVectorizedLoop(VPTransformState &State) { | |||||
| getOrCreateVectorTripCount(nullptr), LoopMiddleBlock, State); | ||||||
| } | ||||||
|
|
||||||
| if (!State.Plan->getVectorLoopRegion()) | ||||||
| return; | ||||||
|
|
||||||
|
Comment on lines
+3017
to
+3019
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. Comment why this is placed here, i.e., why all above should work even if vector loop region was removed, and all below should not.
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. Added thanks |
||||||
| for (Instruction *PI : PredicatedInstructions) | ||||||
| sinkScalarOperands(&*PI); | ||||||
|
|
||||||
|
|
@@ -7859,24 +7864,25 @@ DenseMap<const SCEV *, Value *> LoopVectorizationPlanner::executePlan( | |||||
| makeFollowupLoopID(OrigLoopID, {LLVMLoopVectorizeFollowupAll, | ||||||
| LLVMLoopVectorizeFollowupVectorized}); | ||||||
|
|
||||||
| VPBasicBlock *HeaderVPBB = | ||||||
| BestVPlan.getVectorLoopRegion()->getEntryBasicBlock(); | ||||||
| Loop *L = LI->getLoopFor(State.CFG.VPBB2IRBB[HeaderVPBB]); | ||||||
| if (VectorizedLoopID) | ||||||
| L->setLoopID(*VectorizedLoopID); | ||||||
| else { | ||||||
| // Keep all loop hints from the original loop on the vector loop (we'll | ||||||
| // replace the vectorizer-specific hints below). | ||||||
| if (MDNode *LID = OrigLoop->getLoopID()) | ||||||
| L->setLoopID(LID); | ||||||
|
|
||||||
| LoopVectorizeHints Hints(L, true, *ORE); | ||||||
| Hints.setAlreadyVectorized(); | ||||||
| if (auto *R = BestVPlan.getVectorLoopRegion()) { | ||||||
|
||||||
| if (auto *R = BestVPlan.getVectorLoopRegion()) { | |
| if (auto *LoopRegion = BestVPlan.getVectorLoopRegion()) { |
(R is often used to denote Recipe)
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.
| if (auto *R = BestVPlan.getVectorLoopRegion()) { | |
| if (auto *LoopRegion = BestVPlan.getVectorLoopRegion()) { |
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.
Adjusted, 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.
Overwrite ExitVPBB set to middle block above?
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.
Name always refers to the middle block, fixed in 11c6af6, thanks
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -451,7 +451,6 @@ void VPBasicBlock::connectToPredecessors(VPTransformState::CFGState &CFG) { | |
| CFG.DTU.applyUpdates({{DominatorTree::Insert, PredBB, NewBB}}); | ||
| } | ||
| } | ||
|
|
||
| void VPIRBasicBlock::execute(VPTransformState *State) { | ||
| assert(getHierarchicalSuccessors().size() <= 2 && | ||
| "VPIRBasicBlock can have at most two successors at the moment!"); | ||
|
|
@@ -484,7 +483,7 @@ void VPBasicBlock::execute(VPTransformState *State) { | |
| }; | ||
|
|
||
| // 1. Create an IR basic block. | ||
| if (this == getPlan()->getVectorPreheader() || | ||
| if (this == getPlan()->getEntry() || | ||
|
||
| (Replica && this == getParent()->getEntry()) || | ||
| IsReplicateRegion(getSingleHierarchicalPredecessor())) { | ||
| // Reuse the previous basic block if the current VPBB is either | ||
|
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. Update comment below: "// * the vector preheader," >> "// * the entry to VPlan,"
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, thanks |
||
|
|
@@ -939,7 +938,6 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV, | |
|
|
||
| IRBuilder<> Builder(State.CFG.PrevBB->getTerminator()); | ||
| // FIXME: Model VF * UF computation completely in VPlan. | ||
| assert(VFxUF.getNumUsers() && "VFxUF expected to always have users"); | ||
|
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. Is this related?
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. We may remove the users (the canonical IV increment). Updated to account for that in assertion |
||
| unsigned UF = getUF(); | ||
| if (VF.getNumUsers()) { | ||
| Value *RuntimeVF = getRuntimeVF(Builder, TCTy, State.VF); | ||
|
|
@@ -1024,16 +1022,17 @@ void VPlan::execute(VPTransformState *State) { | |
| for (VPBlockBase *Block : RPOT) | ||
| Block->execute(State); | ||
|
|
||
| VPBasicBlock *LatchVPBB = getVectorLoopRegion()->getExitingBasicBlock(); | ||
| BasicBlock *VectorLatchBB = State->CFG.VPBB2IRBB[LatchVPBB]; | ||
| if (auto *LoopRegion = getVectorLoopRegion()) { | ||
|
||
| VPBasicBlock *LatchVPBB = LoopRegion->getExitingBasicBlock(); | ||
| BasicBlock *VectorLatchBB = State->CFG.VPBB2IRBB[LatchVPBB]; | ||
|
|
||
| // Fix the latch value of canonical, reduction and first-order recurrences | ||
| // phis in the vector loop. | ||
| VPBasicBlock *Header = getVectorLoopRegion()->getEntryBasicBlock(); | ||
| for (VPRecipeBase &R : Header->phis()) { | ||
| // Skip phi-like recipes that generate their backedege values themselves. | ||
| if (isa<VPWidenPHIRecipe>(&R)) | ||
| continue; | ||
| // Fix the latch value of canonical, reduction and first-order recurrences | ||
| // phis in the vector loop. | ||
| VPBasicBlock *Header = LoopRegion->getEntryBasicBlock(); | ||
| for (VPRecipeBase &R : Header->phis()) { | ||
| // Skip phi-like recipes that generate their backedege values themselves. | ||
| if (isa<VPWidenPHIRecipe>(&R)) | ||
| continue; | ||
|
|
||
| if (isa<VPWidenInductionRecipe>(&R)) { | ||
| PHINode *Phi = nullptr; | ||
|
|
@@ -1060,13 +1059,14 @@ void VPlan::execute(VPTransformState *State) { | |
| continue; | ||
| } | ||
|
|
||
| auto *PhiR = cast<VPHeaderPHIRecipe>(&R); | ||
| bool NeedsScalar = isa<VPScalarPHIRecipe>(PhiR) || | ||
| (isa<VPReductionPHIRecipe>(PhiR) && | ||
| cast<VPReductionPHIRecipe>(PhiR)->isInLoop()); | ||
| Value *Phi = State->get(PhiR, NeedsScalar); | ||
| Value *Val = State->get(PhiR->getBackedgeValue(), NeedsScalar); | ||
| cast<PHINode>(Phi)->addIncoming(Val, VectorLatchBB); | ||
| auto *PhiR = cast<VPHeaderPHIRecipe>(&R); | ||
| bool NeedsScalar = isa<VPScalarPHIRecipe>(PhiR) || | ||
| (isa<VPReductionPHIRecipe>(PhiR) && | ||
| cast<VPReductionPHIRecipe>(PhiR)->isInLoop()); | ||
| Value *Phi = State->get(PhiR, NeedsScalar); | ||
| Value *Val = State->get(PhiR->getBackedgeValue(), NeedsScalar); | ||
| cast<PHINode>(Phi)->addIncoming(Val, VectorLatchBB); | ||
| } | ||
| } | ||
|
|
||
| State->CFG.DTU.flush(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3722,6 +3722,7 @@ class VPRegionBlock : public VPBlockBase { | |||||
| assert(!isReplicator() && "should only get pre-header of loop regions"); | ||||||
| return getSinglePredecessor()->getExitingBasicBlock(); | ||||||
| } | ||||||
| void clearEntry() { Entry = nullptr; } | ||||||
|
|
||||||
| /// An indicator whether this region is to generate multiple replicated | ||||||
| /// instances of output IR corresponding to its VPBlockBases. | ||||||
|
|
@@ -3866,7 +3867,10 @@ class VPlan { | |||||
|
|
||||||
| /// Returns the preheader of the vector loop region. | ||||||
|
||||||
| /// Returns the preheader of the vector loop region. | |
| /// Returns the preheader of the vector loop region, if one exists, or null otherwise. |
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 |
|---|---|---|
|
|
@@ -2378,7 +2378,9 @@ void VPBranchOnMaskRecipe::execute(VPTransformState &State) { | |
| // Replace the temporary unreachable terminator with a new conditional branch, | ||
| // whose two destinations will be set later when they are created. | ||
| auto *CurrentTerminator = State.CFG.PrevBB->getTerminator(); | ||
| assert(isa<UnreachableInst>(CurrentTerminator) && | ||
| assert((isa<UnreachableInst>(CurrentTerminator) || | ||
| (isa<BranchInst>(CurrentTerminator) && | ||
| !CurrentTerminator->getOperand(0))) && | ||
|
||
| "Expected to replace unreachable terminator with conditional branch."); | ||
| auto *CondBr = BranchInst::Create(State.CFG.PrevBB, nullptr, ConditionBit); | ||
| CondBr->setSuccessor(0, nullptr); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -692,16 +692,44 @@ void VPlanTransforms::optimizeForVFAndUF(VPlan &Plan, ElementCount BestVF, | |||||
| !SE.isKnownPredicate(CmpInst::ICMP_ULE, TripCount, C)) | ||||||
| return; | ||||||
|
|
||||||
|
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. Say something about what is about to happen now.
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. Added, thanks |
||||||
| LLVMContext &Ctx = SE.getContext(); | ||||||
| auto *BOC = new VPInstruction( | ||||||
| VPInstruction::BranchOnCond, | ||||||
| {Plan.getOrAddLiveIn(ConstantInt::getTrue(Ctx))}, Term->getDebugLoc()); | ||||||
|
|
||||||
| SmallVector<VPValue *> PossiblyDead(Term->operands()); | ||||||
| Term->eraseFromParent(); | ||||||
| auto *Header = cast<VPBasicBlock>(Plan.getVectorLoopRegion()->getEntry()); | ||||||
| if (any_of(Header->phis(), | ||||||
| IsaPred<VPWidenIntOrFpInductionRecipe, VPReductionPHIRecipe>)) { | ||||||
| LLVMContext &Ctx = SE.getContext(); | ||||||
| auto *BOC = new VPInstruction( | ||||||
| VPInstruction::BranchOnCond, | ||||||
| {Plan.getOrAddLiveIn(ConstantInt::getTrue(Ctx))}, Term->getDebugLoc()); | ||||||
| ExitingVPBB->appendRecipe(BOC); | ||||||
| } else { | ||||||
|
||||||
| for (VPRecipeBase &R : make_early_inc_range(Header->phis())) { | ||||||
| auto *P = cast<VPHeaderPHIRecipe>(&R); | ||||||
|
||||||
| auto *P = cast<VPHeaderPHIRecipe>(&R); | |
| auto *HeaderPhiR = cast<VPHeaderPHIRecipe>(&R); |
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.
Potentially unsafe, may leak if the programmer forgets about it. Maybe, use Destructor or something like RAII?
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.
I'm not sure if there is a nice way to do so currently. The whole replace-and-erase logic in the block needs to happen atomically at the moment. To avoid having to do manual delete's, VPlan could track all blocks added to it and delete them all when the VPlan is deleted (at the moment it only deletes reachable blocks). But that would require some extra logic to make sure all blocks are added properly.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,15 +80,13 @@ define void @powi_call(ptr %P) { | |
| ; CHECK: [[VECTOR_PH]]: | ||
| ; CHECK-NEXT: br label %[[VECTOR_BODY:.*]] | ||
| ; CHECK: [[VECTOR_BODY]]: | ||
| ; CHECK-NEXT: [[INDEX:%.*]] = phi i64 [ 0, %[[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], %[[VECTOR_BODY]] ] | ||
| ; CHECK-NEXT: [[TMP0:%.*]] = add i64 [[INDEX]], 0 | ||
| ; CHECK-NEXT: [[TMP1:%.*]] = getelementptr inbounds double, ptr [[P]], i64 [[TMP0]] | ||
| ; CHECK-NEXT: [[TMP1:%.*]] = getelementptr inbounds double, ptr [[P]], i64 0 | ||
| ; CHECK-NEXT: [[TMP2:%.*]] = getelementptr inbounds double, ptr [[TMP1]], i32 0 | ||
| ; CHECK-NEXT: [[WIDE_LOAD:%.*]] = load <2 x double>, ptr [[TMP2]], align 8 | ||
| ; CHECK-NEXT: [[TMP3:%.*]] = call <2 x double> @llvm.powi.v2f64.i32(<2 x double> [[WIDE_LOAD]], i32 3) | ||
| ; CHECK-NEXT: store <2 x double> [[TMP3]], ptr [[TMP2]], align 8 | ||
| ; CHECK-NEXT: [[INDEX_NEXT]] = add nuw i64 [[INDEX]], 2 | ||
| ; CHECK-NEXT: br i1 true, label %[[MIDDLE_BLOCK:.*]], label %[[VECTOR_BODY]], !llvm.loop [[LOOP4:![0-9]+]] | ||
| ; CHECK-NEXT: [[TMP4:%.*]] = getelementptr inbounds double, ptr [[TMP1]], i32 0 | ||
|
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. Better (continue to) reuse TMP2 instead of replicating it?
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. Yes, this is a fall-out from not running non-VPlan-based simple CSE in fixVectorizedLoop |
||
| ; CHECK-NEXT: store <2 x double> [[TMP3]], ptr [[TMP4]], align 8 | ||
| ; CHECK-NEXT: br label %[[MIDDLE_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. Potential for merging original latch block with middle block?
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. Yep |
||
| ; CHECK: [[MIDDLE_BLOCK]]: | ||
| ; CHECK-NEXT: br i1 true, label %[[EXIT:.*]], label %[[SCALAR_PH]] | ||
| ; CHECK: [[SCALAR_PH]]: | ||
|
|
@@ -102,7 +100,7 @@ define void @powi_call(ptr %P) { | |
| ; CHECK-NEXT: store double [[POWI]], ptr [[GEP]], align 8 | ||
| ; CHECK-NEXT: [[IV_NEXT]] = add i64 [[IV]], 1 | ||
| ; CHECK-NEXT: [[EC:%.*]] = icmp eq i64 [[IV]], 1 | ||
| ; CHECK-NEXT: br i1 [[EC]], label %[[EXIT]], label %[[LOOP]], !llvm.loop [[LOOP5:![0-9]+]] | ||
| ; CHECK-NEXT: br i1 [[EC]], label %[[EXIT]], label %[[LOOP]], !llvm.loop [[LOOP4:![0-9]+]] | ||
| ; CHECK: [[EXIT]]: | ||
| ; CHECK-NEXT: ret void | ||
| ; | ||
|
|
@@ -233,6 +231,5 @@ declare i64 @llvm.fshl.i64(i64, i64, i64) | |
| ; CHECK: [[META1]] = !{!"llvm.loop.isvectorized", i32 1} | ||
| ; CHECK: [[META2]] = !{!"llvm.loop.unroll.runtime.disable"} | ||
| ; CHECK: [[LOOP3]] = distinct !{[[LOOP3]], [[META2]], [[META1]]} | ||
| ; CHECK: [[LOOP4]] = distinct !{[[LOOP4]], [[META1]], [[META2]]} | ||
| ; CHECK: [[LOOP5]] = distinct !{[[LOOP5]], [[META2]], [[META1]]} | ||
| ; CHECK: [[LOOP4]] = distinct !{[[LOOP4]], [[META2]], [[META1]]} | ||
| ;. | ||
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.
Passing in
VectorPHseems fine, still need "FIXME"? In fact, can pass bothVectorDestinationandScalarDestination, i.e., the two block that the new check block is introduced to select between.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.
Fixme is gone, thanks