-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[VPlan] Remove ResumePhi opcode, use regular PHI instead (NFC). #140405
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 1 commit
063d70f
f5ee27b
74eddb5
61779cf
801b98d
069767b
d3df2c3
3f3f002
a02b1d5
0b39926
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2381,8 +2381,8 @@ void InnerLoopVectorizer::introduceCheckBlockInVPlan(BasicBlock *CheckIRBB) { | |||||
| // We just connected a new block to the scalar preheader. Update all | ||||||
| // ResumePhis by adding an incoming value for it, replicating the last value. | ||||||
| for (VPRecipeBase &R : *cast<VPBasicBlock>(ScalarPH)) { | ||||||
|
||||||
| auto *ResumePhi = dyn_cast<VPInstruction>(&R); | ||||||
| if (!ResumePhi || ResumePhi->getOpcode() != VPInstruction::ResumePhi) | ||||||
| auto *ResumePhi = cast<VPPhi>(&R); | ||||||
| if (ResumePhi->getNumIncoming() == ScalarPH->getNumPredecessors()) | ||||||
|
||||||
| continue; | ||||||
| ResumePhi->addOperand( | ||||||
| ResumePhi->getOperand(ResumePhi->getNumOperands() - 1)); | ||||||
|
|
@@ -2533,7 +2533,8 @@ BasicBlock *InnerLoopVectorizer::emitMemRuntimeChecks(BasicBlock *Bypass) { | |||||
| static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) { | ||||||
| VPIRBasicBlock *IRVPBB = VPBB->getPlan()->createVPIRBasicBlock(IRBB); | ||||||
| for (auto &R : make_early_inc_range(*VPBB)) { | ||||||
| assert(!R.isPhi() && "Tried to move phi recipe to end of block"); | ||||||
| assert((IRVPBB->empty() || IRVPBB->back().isPhi() || !R.isPhi()) && | ||||||
| "Tried to move phi recipe to end of block"); | ||||||
|
||||||
| "Tried to move phi recipe to end of block"); | |
| "Tried to move phi recipe after a non-phi 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.
Updated 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.
Can this become
| assert(count_if(EpiRedResult->users(), IsResumePhi) == 1 && | |
| assert(count_if(EpiRedResult->users(), IsaPred<VPPhi>) == 1 && |
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.
Yep, updated thanks
| 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). | ||
|
Comment on lines
-890
to
-893
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. Worth retaining this documentation somewhere, possibly updated 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. This comment wasn't accruate any longer; they now have incoming values for all predecssors, hence being 'full' phis. |
||
| ResumePhi, | ||
| CalculateTripCountMinusVF, | ||
| // Increment the canonical IV separately for each unrolled part. | ||
| CanonicalIVIncrementForPart, | ||
|
|
@@ -1137,11 +1132,15 @@ struct VPPhi : public VPInstruction, public VPPhiAccessors { | |
| VPPhi(ArrayRef<VPValue *> Operands, DebugLoc DL, const Twine &Name = "") | ||
| : VPInstruction(Instruction::PHI, Operands, DL, Name) {} | ||
|
|
||
| static inline bool classof(const VPRecipeBase *U) { | ||
| static inline bool classof(const VPUser *U) { | ||
| auto *R = dyn_cast<VPInstruction>(U); | ||
| return R && R->getOpcode() == Instruction::PHI; | ||
| } | ||
|
|
||
| VPPhi *clone() override { | ||
| return new VPPhi(operands(), getDebugLoc(), getName()); | ||
| } | ||
|
|
||
| void execute(VPTransformState &State) override; | ||
|
|
||
| #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -732,17 +732,6 @@ Value *VPInstruction::generate(VPTransformState &State) { | |||||
| Value *Addend = State.get(getOperand(1), VPLane(0)); | ||||||
| return Builder.CreatePtrAdd(Ptr, Addend, Name, getGEPNoWrapFlags()); | ||||||
| } | ||||||
| case VPInstruction::ResumePhi: { | ||||||
| auto *NewPhi = | ||||||
| Builder.CreatePHI(State.TypeAnalysis.inferScalarType(this), 2, Name); | ||||||
| for (const auto &[IncVPV, PredVPBB] : | ||||||
| zip(operands(), getParent()->getPredecessors())) { | ||||||
| Value *IncV = State.get(IncVPV, /* IsScalar */ true); | ||||||
| BasicBlock *PredBB = State.CFG.VPBB2IRBB.at(cast<VPBasicBlock>(PredVPBB)); | ||||||
| NewPhi->addIncoming(IncV, PredBB); | ||||||
| } | ||||||
| return NewPhi; | ||||||
| } | ||||||
| case VPInstruction::AnyOf: { | ||||||
| Value *A = State.get(getOperand(0)); | ||||||
| return Builder.CreateOrReduce(A); | ||||||
|
|
@@ -841,8 +830,7 @@ bool VPInstruction::isVectorToScalar() const { | |||||
| } | ||||||
|
|
||||||
| bool VPInstruction::isSingleScalar() const { | ||||||
| return getOpcode() == VPInstruction::ResumePhi || | ||||||
| getOpcode() == Instruction::PHI; | ||||||
| return getOpcode() == Instruction::PHI; | ||||||
| } | ||||||
|
|
||||||
| void VPInstruction::execute(VPTransformState &State) { | ||||||
|
|
@@ -928,7 +916,6 @@ bool VPInstruction::onlyFirstLaneUsed(const VPValue *Op) const { | |||||
| case VPInstruction::CanonicalIVIncrementForPart: | ||||||
| case VPInstruction::BranchOnCount: | ||||||
| case VPInstruction::BranchOnCond: | ||||||
| case VPInstruction::ResumePhi: | ||||||
| return true; | ||||||
| case VPInstruction::PtrAdd: | ||||||
| return Op == getOperand(0) || vputils::onlyFirstLaneUsed(this); | ||||||
|
|
@@ -985,9 +972,6 @@ void VPInstruction::print(raw_ostream &O, const Twine &Indent, | |||||
| case VPInstruction::ActiveLaneMask: | ||||||
| O << "active lane mask"; | ||||||
| break; | ||||||
| case VPInstruction::ResumePhi: | ||||||
| O << "resume-phi"; | ||||||
|
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. Nit: can retain how resume-phis are printed by checking if a phi is in the scalar preheader when printed in this patch, reducing its test diff and keeping it strictly NFC. Changing its printing can be done separately as a follow up.
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. Done thanks |
||||||
| break; | ||||||
| case VPInstruction::ExplicitVectorLength: | ||||||
| O << "EXPLICIT-VECTOR-LENGTH"; | ||||||
| break; | ||||||
|
|
@@ -1095,11 +1079,19 @@ void VPInstructionWithType::print(raw_ostream &O, const Twine &Indent, | |||||
|
|
||||||
| void VPPhi::execute(VPTransformState &State) { | ||||||
| State.setDebugLocFrom(getDebugLoc()); | ||||||
| BasicBlock *VectorPH = State.CFG.VPBB2IRBB.at(getIncomingBlock(0)); | ||||||
| Value *Start = State.get(getIncomingValue(0), VPLane(0)); | ||||||
| PHINode *Phi = State.Builder.CreatePHI(Start->getType(), 2, getName()); | ||||||
| Phi->addIncoming(Start, VectorPH); | ||||||
| State.set(this, Phi, VPLane(0)); | ||||||
| PHINode *NewPhi = State.Builder.CreatePHI( | ||||||
| State.TypeAnalysis.inferScalarType(this), 2, getName()); | ||||||
| // TODO: Fixup incoming values once other recipes are enabled. | ||||||
|
||||||
| // TODO: Fixup incoming values once other recipes are enabled. | |
| // TODO: Fixup all incoming values of scalar preheader phis once recipes defining them are introduced. |
?
Perhaps clearer to first set unsigned NumIncoming = getNumIncoming(); and then override it
if (getParent() == scalar-preheader)
NumIncoming = 1;
with the comment addressing the latter.
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.
Update, although the case which needs fixing up later are the phis in the header blocks, for which the latch value has not yet been generated; the phis in the scalar preheader will only execute once all incoming values have been generated.
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.
it appears not all VPPhis are updated?
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. I removed the checks, that skipped phis in the latest version, they are not needed after landing #140132