-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[VPlan] Introduce ResumePhi VPInstruction, use to create phi for FOR. #94760
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 4 commits
1ea4a7c
bd6fe32
9f53c00
f028ab3
94672b5
3716289
ae77a2a
d19016f
24133b7
5e61cb4
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 | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -607,10 +607,6 @@ class InnerLoopVectorizer { | |||||||||
BasicBlock *MiddleBlock, BasicBlock *VectorHeader, | ||||||||||
VPlan &Plan, VPTransformState &State); | ||||||||||
|
||||||||||
/// Create the phi node for the resume value of first order recurrences in the | ||||||||||
/// scalar preheader and update the users in the scalar loop. | ||||||||||
void fixFixedOrderRecurrence(VPLiveOut *LO, VPTransformState &State); | ||||||||||
|
||||||||||
/// Iteratively sink the scalarized operands of a predicated instruction into | ||||||||||
/// the block that was created for it. | ||||||||||
void sinkScalarOperands(Instruction *PredInst); | ||||||||||
|
@@ -3294,19 +3290,6 @@ void InnerLoopVectorizer::fixVectorizedLoop(VPTransformState &State, | |||||||||
if (EnableVPlanNativePath) | ||||||||||
fixNonInductionPHIs(Plan, State); | ||||||||||
|
||||||||||
// At this point every instruction in the original loop is widened to a | ||||||||||
// vector form. Note that fixing reduction phis, as well as extracting the | ||||||||||
// exit and resume values for fixed-order recurrences are already modeled in | ||||||||||
// VPlan. All that remains to do here is to create a phi in the scalar | ||||||||||
// pre-header for each fixed-order recurrence resume value. | ||||||||||
// TODO: Also model creating phis in the scalar pre-header in VPlan. | ||||||||||
for (const auto &[_, LO] : to_vector(Plan.getLiveOuts())) { | ||||||||||
if (!Legal->isFixedOrderRecurrence(LO->getPhi())) | ||||||||||
continue; | ||||||||||
fixFixedOrderRecurrence(LO, State); | ||||||||||
Plan.removeLiveOut(LO->getPhi()); | ||||||||||
} | ||||||||||
|
||||||||||
// Forget the original basic block. | ||||||||||
PSE.getSE()->forgetLoop(OrigLoop); | ||||||||||
PSE.getSE()->forgetBlockAndLoopDispositions(); | ||||||||||
|
@@ -3343,8 +3326,6 @@ void InnerLoopVectorizer::fixVectorizedLoop(VPTransformState &State, | |||||||||
VectorLoop->getHeader(), Plan, State); | ||||||||||
} | ||||||||||
|
||||||||||
// Fix LCSSA phis not already fixed earlier. Extracts may need to be generated | ||||||||||
// in the exit block, so update the builder. | ||||||||||
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. Good to retain the first sentence:
? 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. Done, thanks! |
||||||||||
State.Builder.SetInsertPoint(State.CFG.ExitBB, | ||||||||||
State.CFG.ExitBB->getFirstNonPHIIt()); | ||||||||||
for (const auto &KV : Plan.getLiveOuts()) | ||||||||||
|
@@ -3374,33 +3355,6 @@ void InnerLoopVectorizer::fixVectorizedLoop(VPTransformState &State, | |||||||||
VF.getKnownMinValue() * UF); | ||||||||||
} | ||||||||||
|
||||||||||
void InnerLoopVectorizer::fixFixedOrderRecurrence(VPLiveOut *LO, | ||||||||||
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. nit (independent): is the second part of the following comment from above along with setting of insertion point still relevant?
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. Moved setting the insert point to VPLiveOut based on the predecessor. |
||||||||||
VPTransformState &State) { | ||||||||||
// Extract the last vector element in the middle block. This will be the | ||||||||||
// initial value for the recurrence when jumping to the scalar loop. | ||||||||||
VPValue *VPExtract = LO->getOperand(0); | ||||||||||
using namespace llvm::VPlanPatternMatch; | ||||||||||
assert(match(VPExtract, m_VPInstruction<VPInstruction::ExtractFromEnd>( | ||||||||||
m_VPValue(), m_VPValue())) && | ||||||||||
"FOR LiveOut expects to use an extract from end."); | ||||||||||
Value *ResumeScalarFOR = State.get(VPExtract, UF - 1, true); | ||||||||||
|
||||||||||
// Fix the initial value of the original recurrence in the scalar loop. | ||||||||||
PHINode *ScalarHeaderPhi = LO->getPhi(); | ||||||||||
auto *InitScalarFOR = | ||||||||||
ScalarHeaderPhi->getIncomingValueForBlock(LoopScalarPreHeader); | ||||||||||
Builder.SetInsertPoint(LoopScalarPreHeader, LoopScalarPreHeader->begin()); | ||||||||||
auto *ScalarPreheaderPhi = | ||||||||||
Builder.CreatePHI(ScalarHeaderPhi->getType(), 2, "scalar.recur.init"); | ||||||||||
for (auto *BB : predecessors(LoopScalarPreHeader)) { | ||||||||||
auto *Incoming = BB == LoopMiddleBlock ? ResumeScalarFOR : InitScalarFOR; | ||||||||||
ScalarPreheaderPhi->addIncoming(Incoming, BB); | ||||||||||
} | ||||||||||
ScalarHeaderPhi->setIncomingValueForBlock(LoopScalarPreHeader, | ||||||||||
ScalarPreheaderPhi); | ||||||||||
Comment on lines
-3391
to
-3392
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. Note that this incoming is set here, rather than by VPLiveIn's fixPhi. 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 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. ScalarPreheaderPhi is being generated by ResumePhi instead, and is being set here as the incoming value of ScalarHeaderPhi - w/o any vector-to-scalar extraction - so can ResumePhi (in its current use) entail a vector-to-scalar extraction (as opposed to reusing it for feeding exit block phis)? 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. At the moment, ResumePhi won't generate any extracts, as it will be fed by the extracted scalar resume value of the FOR. Possibly more accurately to mark ResumePhi as only having its first lane used instead of vector-to-scalar. (Done in latest version) 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. ok. Would be good to simply indicate somehow that its dealing with a single scalar - coming in (from each pred) and going out. 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 an |
||||||||||
ScalarHeaderPhi->setName("scalar.recur"); | ||||||||||
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 setting of scalar header phi's name to "scalar.recur" retained? 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. No at the moment it is left as is. It would need to be done as post-processing, but adjusting the name of the phi in the scalar loop seems a bit inconsistent already (we don't rename any other phis or values in the scalar loop), so IMO removing it makes sense. 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. OK, better to remove it in a separate preparatory patch? 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. Will do ,thanks! 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. Done in 88e9c56 |
||||||||||
} | ||||||||||
|
||||||||||
void InnerLoopVectorizer::sinkScalarOperands(Instruction *PredInst) { | ||||||||||
// The basic block and loop containing the predicated instruction. | ||||||||||
auto *PredBB = PredInst->getParent(); | ||||||||||
|
@@ -8522,6 +8476,58 @@ static void addUsersInExitBlock(VPBasicBlock *HeaderVPBB, Loop *OrigLoop, | |||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
/// Feed a resume value for every FOR from the vector loop to the scalar loop, | ||||||||||
/// if middle block branches to scalar preheader, by introducing ExtractFromEnd | ||||||||||
/// and ResumePhi recipes in each, respectively, and a VPLiveOut which uses the | ||||||||||
/// latter and corresponds to the scalar header. | ||||||||||
static void addLiveOutsForFirstOrderRecurrences(VPlan &Plan) { | ||||||||||
VPRegionBlock *VectorRegion = Plan.getVectorLoopRegion(); | ||||||||||
|
||||||||||
// Start by finding out if middle block branches to scalar preheader. | ||||||||||
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.
Suggested change
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! |
||||||||||
// TODO: Should be replaced by | ||||||||||
// Plan->getScalarLoopRegion()->getSinglePredecessor() in the future once the | ||||||||||
// scalar region is modeled as well. | ||||||||||
VPBasicBlock *ScalarPHVPBB = nullptr; | ||||||||||
auto *MiddleVPBB = cast<VPBasicBlock>(VectorRegion->getSingleSuccessor()); | ||||||||||
for (VPBlockBase *Succ : MiddleVPBB->getSuccessors()) { | ||||||||||
if (isa<VPIRBasicBlock>(Succ)) | ||||||||||
continue; | ||||||||||
assert(!ScalarPHVPBB && "Two candidates for ScalarPHVPBB?"); | ||||||||||
ScalarPHVPBB = cast<VPBasicBlock>(Succ); | ||||||||||
} | ||||||||||
if (!ScalarPHVPBB) | ||||||||||
return; | ||||||||||
|
||||||||||
for (auto &H : VectorRegion->getEntryBasicBlock()->phis()) { | ||||||||||
auto *FOR = dyn_cast<VPFirstOrderRecurrencePHIRecipe>(&H); | ||||||||||
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.
Suggested change
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. Renamed, thanks! |
||||||||||
if (!FOR) | ||||||||||
continue; | ||||||||||
|
||||||||||
VPBuilder B(ScalarPHVPBB); | ||||||||||
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.
Suggested change
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. Renamed thanks! Also hoisted out defining the builders. |
||||||||||
VPBuilder MiddleBuilder(MiddleVPBB); | ||||||||||
// Reset insert point so new recipes are inserted before terminator and | ||||||||||
// condition, if there is either the former or both. | ||||||||||
if (auto *Terminator = MiddleVPBB->getTerminator()) { | ||||||||||
auto *Condition = dyn_cast<VPInstruction>(Terminator->getOperand(0)); | ||||||||||
assert((!Condition || Condition->getParent() == MiddleVPBB) && | ||||||||||
"Condition expected in MiddleVPBB"); | ||||||||||
MiddleBuilder.setInsertPoint(Condition ? Condition : Terminator); | ||||||||||
} | ||||||||||
|
||||||||||
// Extract the resume value and create a new VPLiveOut for it. | ||||||||||
auto *Resume = MiddleBuilder.createNaryOp( | ||||||||||
VPInstruction::ExtractFromEnd, | ||||||||||
{FOR->getBackedgeValue(), | ||||||||||
Plan.getOrAddLiveIn( | ||||||||||
ConstantInt::get(Plan.getCanonicalIV()->getScalarType(), 1))}, | ||||||||||
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. May be clearer to pre-generate a VPValue of 1 and reuse it. 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. Done, thanks! |
||||||||||
{}, "vector.recur.extract"); | ||||||||||
auto *ResumePhiRecipe = | ||||||||||
B.createNaryOp(VPInstruction::ResumePhi, {Resume, FOR->getStartValue()}, | ||||||||||
{}, "scalar.recur.init"); | ||||||||||
Plan.addLiveOut(cast<PHINode>(FOR->getUnderlyingInstr()), ResumePhiRecipe); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
VPlanPtr | ||||||||||
LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) { | ||||||||||
|
||||||||||
|
@@ -8686,6 +8692,8 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) { | |||||||||
"VPBasicBlock"); | ||||||||||
RecipeBuilder.fixHeaderPhis(); | ||||||||||
|
||||||||||
addLiveOutsForFirstOrderRecurrences(*Plan); | ||||||||||
|
||||||||||
// --------------------------------------------------------------------------- | ||||||||||
// Transform initial VPlan: Apply previously taken decisions, in order, to | ||||||||||
// bring the VPlan to its final state. | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -682,7 +682,10 @@ class VPBlockBase { | |||||||
}; | ||||||||
|
||||||||
/// A value that is used outside the VPlan. The operand of the user needs to be | ||||||||
/// added to the associated LCSSA phi node. | ||||||||
/// added to the associated LCSSA phi node. The incoming block from VPlan is | ||||||||
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.
Suggested change
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. Fixed, thanks! |
||||||||
/// determined by where the VPValue is defined: if it is defined by a recipe | ||||||||
/// outside a region, its parent block is used, otherwise the middle block is | ||||||||
/// used. | ||||||||
class VPLiveOut : public VPUser { | ||||||||
PHINode *Phi; | ||||||||
|
||||||||
|
@@ -694,11 +697,8 @@ class VPLiveOut : public VPUser { | |||||||
return U->getVPUserID() == VPUser::VPUserID::LiveOut; | ||||||||
} | ||||||||
|
||||||||
/// Fixup the wrapped LCSSA phi node in the unique exit block. This simply | ||||||||
/// means we need to add the appropriate incoming value from the middle | ||||||||
/// block as exiting edges from the scalar epilogue loop (if present) are | ||||||||
/// already in place, and we exit the vector loop exclusively to the middle | ||||||||
/// block. | ||||||||
/// Fixup the wrapped LCSSA phi node. This means we need to add the | ||||||||
/// appropriate incoming value from the precessor Pred. | ||||||||
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.
Suggested change
Note that a phi ( 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 the wording.
Would be good as follow-up |
||||||||
void fixPhi(VPlan &Plan, VPTransformState &State); | ||||||||
|
||||||||
/// Returns true if the VPLiveOut uses scalars of operand \p Op. | ||||||||
|
@@ -1191,6 +1191,11 @@ 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, | ||||||||
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. Thanks for renaming, worth updating the title and summary of the patch. 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. Title + description should be updated now |
||||||||
CalculateTripCountMinusVF, | ||||||||
// Increment the canonical IV separately for each unrolled part. | ||||||||
CanonicalIVIncrementForPart, | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -193,11 +193,18 @@ void VPLiveOut::fixPhi(VPlan &Plan, VPTransformState &State) { | |||||||||||||||||||||||||
auto Lane = vputils::isUniformAfterVectorization(ExitValue) | ||||||||||||||||||||||||||
? VPLane::getFirstLane() | ||||||||||||||||||||||||||
: VPLane::getLastLaneForVF(State.VF); | ||||||||||||||||||||||||||
VPBasicBlock *MiddleVPBB = | ||||||||||||||||||||||||||
VPBasicBlock *PredVPBB = | ||||||||||||||||||||||||||
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.
Suggested change
|
||||||||||||||||||||||||||
cast<VPBasicBlock>(Plan.getVectorLoopRegion()->getSingleSuccessor()); | ||||||||||||||||||||||||||
BasicBlock *MiddleBB = State.CFG.VPBB2IRBB[MiddleVPBB]; | ||||||||||||||||||||||||||
Phi->addIncoming(State.get(ExitValue, VPIteration(State.UF - 1, Lane)), | ||||||||||||||||||||||||||
MiddleBB); | ||||||||||||||||||||||||||
VPRecipeBase *DefRecipe = ExitValue->getDefiningRecipe(); | ||||||||||||||||||||||||||
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.
Suggested change
can ExitValue be a live in, or can we assert it has a defining recipe. 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. It can be a live in 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. ok, perhaps a live-in feeding a live-out can be useful, e.g., to indicate if the loop was entered(?). 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, but I think this mostly boils down to not having to unnecessarily restrict the inputs. |
||||||||||||||||||||||||||
if (DefRecipe && !DefRecipe->getParent()->getParent()) | ||||||||||||||||||||||||||
PredVPBB = DefRecipe->getParent(); | ||||||||||||||||||||||||||
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. // Values leaving the vector loop reach live out phi's in the exit block via middle block.
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 (with Exit -> Exiting) |
||||||||||||||||||||||||||
BasicBlock *PredBB = State.CFG.VPBB2IRBB[PredVPBB]; | ||||||||||||||||||||||||||
State.Builder.SetInsertPoint(PredBB, PredBB->getFirstNonPHIIt()); | ||||||||||||||||||||||||||
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. Worth adding a comment that insertion point is set in PrefBB for potential generation of instructions to extract Is this potential post-extract why ResumePhi is considered (maybe!) vector-to-scalar? These extracts best materialize into recipes. 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 a comment + TODO, thanks!
Yes, we need to select the last lane, which may require creating a new extract, if the VPValue is a vector. Added a TODO to model extracts explicitly. |
||||||||||||||||||||||||||
Value *V = State.get(ExitValue, VPIteration(State.UF - 1, Lane)); | ||||||||||||||||||||||||||
if (Phi->getBasicBlockIndex(PredBB) != -1) | ||||||||||||||||||||||||||
Phi->setIncomingValueForBlock(PredBB, V); | ||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||
Phi->addIncoming(V, PredBB); | ||||||||||||||||||||||||||
Comment on lines
+210
to
+214
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. This is where we need to figure out which predecessor P corresponds to For live outs that correspond to exit block, P is middle block (which can be retrieved directly from Plan as done now, or from State.CFG.ExitBB, or from State.builder which is (still?) set to ExitBB). Can we check ExitValue's parent to figure out P, in both cases, asserting that its corresponding IRBB is a predecessor of Phi's block? Phi's of exit block know not of middle block, as exit block (and its phi's) is originally connected to scalar loop only. fixPhi()'s documentation above needs update. 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 to use the defining recipe's parent, if it is outside a region, otherwise use MiddleVPBB |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) | ||||||||||||||||||||||||||
|
@@ -303,6 +310,7 @@ bool VPInstruction::canGenerateScalarForFirstLane() const { | |||||||||||||||||||||||||
case VPInstruction::CanonicalIVIncrementForPart: | ||||||||||||||||||||||||||
case VPInstruction::PtrAdd: | ||||||||||||||||||||||||||
case VPInstruction::ExplicitVectorLength: | ||||||||||||||||||||||||||
case VPInstruction::ResumePhi: | ||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||
|
@@ -593,13 +601,35 @@ Value *VPInstruction::generatePerPart(VPTransformState &State, unsigned Part) { | |||||||||||||||||||||||||
Value *Addend = State.get(getOperand(1), Part, /* IsScalar */ true); | ||||||||||||||||||||||||||
return Builder.CreatePtrAdd(Ptr, Addend, Name); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
case VPInstruction::ResumePhi: { | ||||||||||||||||||||||||||
if (Part != 0) | ||||||||||||||||||||||||||
return State.get(this, 0, /*IsScalar*/ true); | ||||||||||||||||||||||||||
Value *IncomingFromVPlanPred = | ||||||||||||||||||||||||||
State.get(getOperand(0), Part, /* IsScalar */ true); | ||||||||||||||||||||||||||
Value *IncomingFromOtherPreds = | ||||||||||||||||||||||||||
State.get(getOperand(1), Part, /* IsScalar */ true); | ||||||||||||||||||||||||||
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 it conceivable for the scalar preheader to have a single predecessor, namely, VPlanPred/middle-block? I.e., when trip count is known to be larger that VFxUF and no (other) runtime checks are needed to bypass the vector loop? 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 that should be possible, but it should be handled correctly (generating a phi with a single incoming value from the VPlan predecessor) 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. ok, the second operand (FOR->getStartValue()) is redundant in this case, but exists - so getOperand(1) works. 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 |
||||||||||||||||||||||||||
auto *NewPhi = | ||||||||||||||||||||||||||
Builder.CreatePHI(IncomingFromOtherPreds->getType(), 2, Name); | ||||||||||||||||||||||||||
BasicBlock *VPlanPred = | ||||||||||||||||||||||||||
State.CFG | ||||||||||||||||||||||||||
.VPBB2IRBB[cast<VPBasicBlock>(getParent()->getSinglePredecessor())]; | ||||||||||||||||||||||||||
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. Note that since middle block is a VPIRBasicBlock, its corresponding IRBB can be obtained directly via getIRBasicBlock(). |
||||||||||||||||||||||||||
NewPhi->addIncoming(IncomingFromVPlanPred, VPlanPred); | ||||||||||||||||||||||||||
for (auto *OtherPred : predecessors(Builder.GetInsertBlock())) { | ||||||||||||||||||||||||||
assert(OtherPred != VPlanPred && | ||||||||||||||||||||||||||
"VPlan predecessors should not be connected yet"); | ||||||||||||||||||||||||||
NewPhi->addIncoming(IncomingFromOtherPreds, OtherPred); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Comment on lines
+665
to
+670
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.
Suggested change
a bit more logical to first connect all existing predecessors asserting to not be VPlanPred, and then connect the latter(?) 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. Agreed, but this would require updating a bunch of tests due to different incoming order. Better to be done as follow up? 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. Sure 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. Just had a look at this and this would mean we have different incoming orders for the induction resume values and FOR resume values until we moved induction resume values to also use ResumePhi. WDYT about only adjusting the order once both use ResumePhi? |
||||||||||||||||||||||||||
return NewPhi; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||
llvm_unreachable("Unsupported opcode for instruction"); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
bool VPInstruction::isVectorToScalar() const { | ||||||||||||||||||||||||||
return getOpcode() == VPInstruction::ExtractFromEnd || | ||||||||||||||||||||||||||
getOpcode() == VPInstruction::ResumePhi || | ||||||||||||||||||||||||||
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. still needed? 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. Removed, superseded by isSingleScalar. |
||||||||||||||||||||||||||
getOpcode() == VPInstruction::ComputeReductionResult; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
@@ -729,6 +759,9 @@ void VPInstruction::print(raw_ostream &O, const Twine &Indent, | |||||||||||||||||||||||||
case VPInstruction::ActiveLaneMask: | ||||||||||||||||||||||||||
O << "active lane mask"; | ||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
case VPInstruction::ResumePhi: | ||||||||||||||||||||||||||
O << "exit-phi"; | ||||||||||||||||||||||||||
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.
Suggested change
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! |
||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
case VPInstruction::ExplicitVectorLength: | ||||||||||||||||||||||||||
O << "EXPLICIT-VECTOR-LENGTH"; | ||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -937,22 +937,16 @@ bool VPlanTransforms::adjustFixedOrderRecurrences(VPlan &Plan, | |
Type *IntTy = Plan.getCanonicalIV()->getScalarType(); | ||
|
||
// Extract the penultimate value of the recurrence and update VPLiveOut | ||
// users of the recurrence splice. | ||
// users of the recurrence splice. Note that the extract of the final value | ||
// used to resume in the scalar loop is created earlier during VPlan | ||
// construction. | ||
auto *Penultimate = cast<VPInstruction>(MiddleBuilder.createNaryOp( | ||
VPInstruction::ExtractFromEnd, | ||
{FOR->getBackedgeValue(), | ||
Plan.getOrAddLiveIn(ConstantInt::get(IntTy, 2))}, | ||
{}, "vector.recur.extract.for.phi")); | ||
RecurSplice->replaceUsesWithIf( | ||
Penultimate, [](VPUser &U, unsigned) { return isa<VPLiveOut>(&U); }); | ||
|
||
// Extract the resume value and create a new VPLiveOut for it. | ||
auto *Resume = MiddleBuilder.createNaryOp( | ||
VPInstruction::ExtractFromEnd, | ||
{FOR->getBackedgeValue(), | ||
Plan.getOrAddLiveIn(ConstantInt::get(IntTy, 1))}, | ||
{}, "vector.recur.extract"); | ||
Plan.addLiveOut(cast<PHINode>(FOR->getUnderlyingInstr()), Resume); | ||
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. Note that this live-out corresponds to the scalar header and is fed a value (Resume) created in the middle block, i.e., w/o going through the scalar preheader. It is currently wired explicitly, rather than via VPLiveOut::fixPhi(), which as documented deals only with wiring directly to middle block live-outs that correspond to the exit block. Worth leaving behind a comment that the s_resume value is created in the middle block and fed into the scalar header via the scalar preheader, at VPlan construction. 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 a comment, thanks! 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. Extended the comment above, thanks! |
||
} | ||
return true; | ||
} | ||
|
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.
There's more to remove here...
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.
Of course, removed, thanks!