[PATCH] D123537: [VPlan] Model first exit values using VPLiveOut.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 20 01:43:28 PDT 2022
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:591
- /// already in place, and we exit the vector loop exclusively to the middle
- /// block.
- void fixLCSSAPHIs(VPTransformState &State);
----------------
Ayal wrote:
> Worth retaining this explanation when documenting LiveOut/fixPhi()?
I moved the comment to `VPLiveOut::fixPhi`.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3394
PHI->addIncoming(I.second, MiddleBlock);
+ Plan.removeLiveOut(PHI);
}
----------------
Ayal wrote:
> Suffice to remove LiveOut only when actually fixing the incoming, i.e., under the if?
Yes that should be sufficient, updated!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3709
+ } else {
+ // Otherwise all exit values should be forwarded to the scalar loop earlier.
+ Plan.clearLiveOuts();
----------------
Ayal wrote:
> I.e., there’s nothing to fix from vector loop, phi should have incoming from scalar loop only?
Reworded the comment, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3710
+ // Otherwise all exit values should be forwarded to the scalar loop earlier.
+ Plan.clearLiveOuts();
}
----------------
Ayal wrote:
> Worth reversing the condition to handle the simpler case first?
>
> Or have addUsersInExitBlock() bail out early if requiresScalarEpilogue()?
> Worth reversing the condition to handle the simpler case first?
Done!
> Or have addUsersInExitBlock() bail out early if requiresScalarEpilogue()?
I think that would have the drawback that we might miss uses outside the vector loop in VPlan analyses. The latest version of the patch removed a workaround that required us checking the IR def-use chains to detect outside users.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3715
+ // in the exit block, so update the builder.
+ State.Builder.SetInsertPoint(State.CFG.ExitBB, State.CFG.ExitBB->begin());
+ for (auto &KV : Plan.getLiveOuts())
----------------
Ayal wrote:
> Set to first non phi instead of begin, as we prepare for non phi extracts, although middle block is free of (LCSSA) phi’s atm?
>
> May as well pass MiddleBlock to fixPhi, having directed Builder to it, instead of having each fixPhi derive it from Plan? (Can also have fixPhi derive MiddleBlock from Builder…)
> Set to first non phi instead of begin, as we prepare for non phi extracts, although middle block is free of (LCSSA) phi’s atm?
Done thanks!
> May as well pass MiddleBlock to fixPhi, having directed Builder to it, instead of having each fixPhi derive it from Plan? (Can also have fixPhi derive MiddleBlock from Builder…)
I updated fixPhi to use the current block from the builder instead, seemed simpler.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3870
+ State.Plan->removeLiveOut(&LCSSAPhi);
+ }
}
----------------
Ayal wrote:
> Single else placed earlier suffices, no need for same else here as well?
I think I might be missing something. Are you referring to removing all live outs earlier in `fixVectorizedLoop` via `Plan.clearLiveOuts`? This should only cover the case where we require scalar epilogues, whereas here we only deal with the other case.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3928
// and so use the select value for the phi instead of the old
- // LoopExitValue.
+ // LoopLiveOut.
if (PreferPredicatedReductionSelect ||
----------------
Ayal wrote:
> In general LiveOut exits from VPlan to IR rather than from Loop. Distinction more important for outerloop vectorization.
I'll undo the change, this was just an accidental replacement.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4056
+ State.Plan->removeLiveOut(&LCSSAPhi);
+ }
----------------
Ayal wrote:
> A bit confusing when to else cleanup LiveOuts; perhaps do so directly rather than as an else, or when creating LiveOuts?
I'm not entirely sure what you are suggesting here. The reason this is done here to only remove the live-out of the current reduction. Doing so when creating LiveOuts is more complicated (as in earlier versions of the patch).
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8717
+ // Only handle single-exit loops for now.
+ if (!ExitBB->getSinglePredecessor())
+ return;
----------------
Ayal wrote:
> Can fold both early exits into one.
>
> Also early exit if requiresScalarEpilogue()?
> Can fold both early exits into one.
Done, thanks!
> Also early exit if requiresScalarEpilogue()?
I think we should still create live-outs if `requiresScalarEpilogue`, otherwise some recipes may be considered dead if they only have uses outside the loop. The latest version of the patch uses that property to remove a workaround in VPlanTransforms that used IR def use chains to detect users outside the loop.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9167
// ReductionOperations are orders top-down from the phi's use to the
- // LoopExitValue. We keep a track of the previous item (the Chain) to tell
+ // LoopLiveOut. We keep a track of the previous item (the Chain) to tell
// which of the two operands will remain scalar and which will be reduced.
----------------
Ayal wrote:
> ditto regarding retention of LoopExitValue vs. VPlanLiveOut.
Changed back thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10671
// updated before vectorising the epilogue loop.
- VPBasicBlock *Header =
- BestEpiPlan.getVectorLoopRegion()->getEntryBasicBlock();
+ VPBasicBlock *Header = VectorLoop->getEntryBasicBlock();
for (VPRecipeBase &R : Header->phis()) {
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > change independent of this patch?
> > Done in 5b00d13c0071.
> While we're here, first set Header then set its name?
Done in fcfb86483b29df124c0b4a61ff65b0c6800f64b7
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10683
+ // Update exit values for LCSSA phis in the exit block for the epilogue
+ // loop.
+ VPBasicBlock *MiddleVPBB =
----------------
Ayal wrote:
> How do the new exit values created by addUsersInExitBlock() below differ from the existing exit values of BestEpiPlan they replace? Both wrap the same LCSSA phis and presumably use the same VPValue from within BestEpiPlan. When called to fix their phi, they should update distinct incoming values corresponding to distinct middle blocks, but those are independent of the exit values themselves - that's why Plan is provided to fixPhi()?
>
I think the comment was for an earlier version, the code is gone now. Depends on D125810
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10692
+ DeadInstructions);
+
LVP.executePlan(EPI.EpilogueVF, EPI.EpilogueUF, BestEpiPlan, EpilogILV,
----------------
Ayal wrote:
> This processLoop() method is becoming 388 LOC long... any change to it should consider refactoring?
I think this comment was for an earlier version of the patch, the changes are gone now
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:651
+void VPLiveOut::fixPhi(VPlan &Plan, VPTransformState &State) {
+ auto Lane = VPLane::getLastLaneForVF(State.VF);
+ if (Plan.isUniformAfterVectorization(getOperand(0)))
----------------
Ayal wrote:
> nits: set ExitingVPValue to operand(0), used twice.
> Derive and set MiddleVPBock from loop/outermost region enclosing ExitingVPValue, rather than from Plan? Or OTOH have it passed in, as it’s the same for all phis to fix.
> nits: set ExitingVPValue to operand(0), used twice.
Done, thanks!
> Derive and set MiddleVPBock from loop/outermost region enclosing ExitingVPValue, rather than from Plan? Or OTOH have it passed in, as it’s the same for all phis to fix.
Updated to get the block from the builder directly.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1103
+void VPlan::addLiveOut(PHINode *PN, VPValue *V) {
+ assert(LiveOuts.find(PN) == LiveOuts.end() &&
+ "an exit value for PN already exists");
----------------
Ayal wrote:
> assert count is zero?
adjusted, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1087
+ for (VPExitValue *EV : ExitValues) {
+ O << "Live-out ";
+ EV->getPhi()->printAsOperand(O);
----------------
Ayal wrote:
> Perhaps call them VPLiveOut/VPLiveOutIR/VPLiveOutUser/VPLiveOutIRUser rather than VPExitValue, given how they are printed, and to complement live-in IR Values which are represented via VPDef-less VPValues supporting getLiveInIRValue()?
>
> assert/verify/note somewhere that VPExitValue is assumed to have a single operand?
Renamed to VPLiveOut &LiveOut.
I added a check to the VPlan verifier.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:818
+ if (getParent()->getPlan()->isUniformAfterVectorization(getOperand(0)))
+ Lane = VPLane::getFirstLane();
+ ExitPhi->addIncoming(
----------------
Ayal wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > Indicate that ExitValue usesScalars()?
> > > > Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.
> > > > Indicate that ExitValue usesScalars()?
> > >
> > > Added `usesScalars` implementation.
> > >
> > > > Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.
> > >
> > > Good point, not sure what the ideal name would be. Changed `ScalarExitValue` but I am not sure if that is much better. They model all exit values that do not need special handling.
> > >>Indicate that ExitValue usesScalars()?
> > > Added usesScalars implementation.
> > It actually usesFirstOrLastLane only... perhaps something to think of in the future.
> >> Perhaps worth emphasizing in the name - reduction epilogues using vectors probably deserve a separate "Exit Value" recipe.
>
> > Good point, not sure what the ideal name would be. Changed ScalarExitValue but I am not sure if that is much better. They model all exit values that do not need special handling.
>
> Hmm, it seems ScalarExitValue merely models/wraps an IR loop-closed-ssa-phi taking care only of hooking it's operand. Perhaps deserves a "VPLiveOut" or "VPLoopClosedPhi" subclass of VPUser?
Do you mean in addition to `VPScalarExitValue`? I've not added a new subclass in this patch as perhaps it would be good add it once we have multiple sub-classes?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:685
+
+ PHINode *getPhi() { return Phi; }
+};
----------------
Ayal wrote:
> const?
> (Currently used for printing only.)
Adjusted, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2629
addVPValue(V);
- return getVPValue(V);
+ return getVPValue(V, OverrideAllowed);
}
----------------
Ayal wrote:
> Change related to this patch?
> Can a test demonstrate its need?
I forgot to remove this change, it's not needed any longer. Removed, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:211
+ }
return true;
}
----------------
Ayal wrote:
> nit: empty line before final return true?
Adjusted, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123537/new/
https://reviews.llvm.org/D123537
More information about the llvm-commits
mailing list