[PATCH] D105008: [VPlan] Add recipe for first-order rec phis, make splicing explicit.
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 5 15:01:44 PDT 2021
Ayal added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4136
fixReduction(cast<VPReductionPHIRecipe>(PhiR->getDef()), State);
- } else if (Legal->isFirstOrderRecurrence(OrigPhi))
+ else if (isa<VPFirstOrderRecurrencePHIRecipe>(PhiR->getDef()))
fixFirstOrderRecurrence(PhiR, State);
----------------
no need to check if !PhiR, no need to getDef()
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4166
// previous iteration. In the first phase of vectorization, we created a
// temporary value for s1. We now complete the vectorization and produce the
// shorthand vector IR shown below (for VF = 4, UF = 1).
----------------
temporary value for s1 - this value is no longer temporary, but rather one that is updated here
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4217
+ VecPhi->setName("vector.recur");
VecPhi->addIncoming(VectorInit, LoopVectorPreHeader);
----------------
Can have FORRecipe::execute() name the phi and hook it up to VectorInit (Start)?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4220
// Fix the latch value of the new recurrence in the vector loop.
+ Value *Incoming = UF == 0 ? VecPhi : State.get(PreviousDef, UF - 1);
VecPhi->addIncoming(Incoming, LI->getLoopFor(LoopVectorBody)->getLoopLatch());
----------------
UF is at-least 1?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9106
VPRecipeBase *Target = RecipeBuilder.getRecipe(Entry.second);
auto *TargetRegion = GetReplicateRegion(Target);
----------------
(lambda taken out of loop, to be reused below)
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9152
}
+ for (VPRecipeBase &R : Plan->getEntry()->getEntryBasicBlock()->phis()) {
----------------
Add a comment what the code below does?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9154
+ for (VPRecipeBase &R : Plan->getEntry()->getEntryBasicBlock()->phis()) {
+ auto *RecurP = dyn_cast<VPFirstOrderRecurrencePHIRecipe>(R.getVPValue());
+ if (!RecurP)
----------------
drop getVPValue()?
RecurP >> RecurPHI?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9159
+ VPRecipeBase *Prev =
+ cast<VPRecipeBase>(RecurP->getBackedgeValue()->getDef());
+ auto *RecurSplice = cast<VPInstruction>(
----------------
BackedgeValue is always a recipe; maybe worth having getBackedgeRecipe()
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9162
+ Builder.createNaryOp(VPInstruction::FirstOrderRecurrenceSplice,
+ {RecurP, Prev->getVPValue()}));
+ if (auto *Region = GetReplicateRegion(Prev)) {
----------------
Better have a `VPValue` *Prev rather than getDef()->getVP[Single]Value()?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9163
+ {RecurP, Prev->getVPValue()}));
+ if (auto *Region = GetReplicateRegion(Prev)) {
+ VPBasicBlock *Succ = cast<VPBasicBlock>(Region->getSingleSuccessor());
----------------
assert Prev is the last insn in its region?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9165
+ VPBasicBlock *Succ = cast<VPBasicBlock>(Region->getSingleSuccessor());
+ RecurSplice->moveBefore(*Succ, Succ->begin());
+ } else
----------------
moveBefore first non-phi?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9169
+ RecurP->replaceAllUsesWith(RecurSplice);
+ RecurSplice->setOperand(0, RecurP);
+ }
----------------
RecurSplice was built with RecurP as it's first operand?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1138
+ // directly.
+ // TODO: Remove once all VPWidenPHIRecipe instances keep all relevant incoming
+ // values as VPValues.
----------------
TODO needed here?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:782
ActiveLaneMask,
+ FirstOrderRecurrenceSplice, // Combines the incoming and previous values of
+ // a first-order recurrence.
----------------
try to maintain lexicographic order?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1132
+ VPFirstOrderRecurrencePHISC, Phi) {
+ addOperand(&Start);
+ }
----------------
another constructor for VPWidenPHIRecipe, receiving both Phi and Start, in addition to the two SC's?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:365
/// Returns the VPValue with index \p I defined by the VPDef.
- VPValue *getVPValue(unsigned I) {
+ VPValue *getVPValue(unsigned I = 0) {
assert(DefinedValues[I] && "defined value must be non-null");
----------------
If we expect a single value, call getVPSingleValue?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105008/new/
https://reviews.llvm.org/D105008
More information about the llvm-commits
mailing list