[PATCH] D115953: [VPlan] Introduce recipe to build scalar steps.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 21 06:22:09 PST 2022
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2701
+ // create the phi node, we will splat the scalar induction variable in each
+ // loop iteration.
+ createVectorIntOrFpInductionPHI(ID, Step, Start, EntryVal, Def, State);
----------------
Ayal wrote:
> By dropping "if (Def->needsVectorIV())" we create a vector IV even if one is (known at this point to) not be needed - all users want scalar values. This case will be cleaned up later, right?
Updated the comment, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8530
-VPWidenIntOrFpInductionRecipe *VPRecipeBuilder::tryToOptimizeInductionTruncate(
+VPRecipeBase *VPRecipeBuilder::tryToOptimizeInductionTruncate(
TruncInst *I, ArrayRef<VPValue *> Operands, VFRange &Range,
----------------
Ayal wrote:
> Is this change still needed or can tryToOptimizeInductionTruncate() continue to return VPWidenIntOrFpInductionRecipe?
It's a leftover from earlier iterations, it is not needed. I removed it, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9715
+void VPScalarIVStepsRecipe::execute(VPTransformState &State) {
+ // Fast-math-flags propagate from the original induction instruction.
----------------
Ayal wrote:
> This is to run once, i.e., not per part/lane inside a replicating region - assert(!State.Instance && "..."); as above.
Added assert, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8426
+ ++NewInsertionPoint;
+ Builder.setInsertPoint(Builder.getInsertBlock(), NewInsertionPoint);
+
----------------
Ayal wrote:
> Update comment above, as block in mask may no longer be the first non-phi instruction in the block?
> This suggests that VPScalarIVStepsRecipes should perhaps be introduced (as first non-phi insns) by a VPlan-to-VPlan transformation after an initial VPlan is formed with ICmpULE/ActiveLaneMask (as initial first non-phi insns) and other 'initial' recipes.
Removed in the latest version
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8934
+
+ if (onlyScalarStepsNeeded(ScalarStepsForI, Range)) {
+ const SCEV *StepSCEV = II->getStep();
----------------
Ayal wrote:
> early-exit if !OnlyScalarStepsNeeded()
The whole function is gone now.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8943
+ else
+ Step = new VPExpandSCEVRecipe(II->getStep(), *PSE.getSE());
+
----------------
Ayal wrote:
> Pass StepSCEV instead of II->getSteps()?
>
> This VPExpandSCEVRecipe needs to be placed somewhere by every caller of tryToBuildScalarSteps.
The whole function is gone now.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8948
+ return new VPScalarIVStepsRecipe(Phi, *II, ScalarStepsForI,
+ Plan.getCanonicalIV(), Step);
+ }
----------------
Ayal wrote:
> Place ScalarStepsForI as last parameter? Can always pass it and have the constructor check if it's equal to Phi or else should be considered a Trunc.
The whole function is gone now.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9115
+ DenseMap<Value *, VPValue *> VectorIVs;
+ DenseMap<Value *, VPValue *> ScalarIVs;
+ SmallVector<VPRecipeBase *> MoveAfterPhis;
----------------
Ayal wrote:
> VectorIVs and ScalarIVs seem useless?
Those changes are gone now.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9145
+ if (auto *Steps =
+ RecipeBuilder.tryToBuildScalarSteps(Instr, Instr, Range, *Plan)) {
+ Plan->addVPValue(Instr, Steps);
----------------
Ayal wrote:
> Rather than pass Instr twice, pass it once as the "Phi" and null as the potential Trunc?
Those changes are gone now.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9151
+ dyn_cast_or_null<VPRecipeBase>(Steps->getOperand(1)->getDef()))
+ MoveAfterPhis.push_back(StepR);
+ continue;
----------------
Ayal wrote:
> This attempt to build a VPScalarIVStepsRecipe given a Phi Instr should be placed inside tryToCreateWidenRecipe() along with its general handling of a Phi Instrs, rather be treated here. The fact that tryToBuildScalarSteps() generates two recipes, returning one (VPScalarIVStepsRecipes) which uses the other (VPExpandSCEVRecipe), where only a single recipe is traditionally created and returned per each Instr, can be dealt with later. In any case the actual placement of both VPScalarIVStepsRecipe
> and VPExpandSCEVRecipe takes place later.
>
> Having both "Steps" and "StepR" may be confusing.
Those changes are gone now.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9174
+ RecipeBuilder.setRecipe(Instr, Recipe);
if (isa<VPWidenIntOrFpInductionRecipe>(Recipe) &&
----------------
Ayal wrote:
> Why is this setRecipe moved?
changes gone in latest iteration
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9190
+ if (auto *StepR = dyn_cast_or_null<VPRecipeBase>(Step->getDef()))
+ MoveAfterPhis.push_back(StepR);
+ continue;
----------------
Ayal wrote:
> (This records both VPExpandSCEVRecipe and its VPScalarIVStepsRecipe created by tryToCreateWidenRecipe() for Trunc Instr., to be placed later.)
changes gone in latest iteration
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:560
+ /// The induction variable of the old basic block.
+ PHINode *OldInduction = nullptr;
+
----------------
Ayal wrote:
> OldInduction moved from protected to public (in some version?) - suffice to provide a const public method to retrieve OldInduction?
OldInduction is not needed in the latest version.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2776
+ assert(!shouldScalarizeInstruction(EntryVal));
+ createVectorIntOrFpInductionPHI(ID, Step, Start, EntryVal, Def, State);
Value *ScalarIV = CreateScalarIV(Step);
----------------
Ayal wrote:
> Can be folded with above call to createVectorIntOrFpInductionPHI(), followed by
>
> ```
> if (NeedsScalarIV) {
> Value *ScalarIV = CreateScalarIV(Step);
> buildScalarSteps(ScalarIV, Step, EntryVal, ID, Def, State);
> }
> ```
> ?
Folded, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7935
State.VectorTripCount = ILV.getOrCreateVectorTripCount(nullptr);
+ State.OldInduction = ILV.OldInduction;
ILV.collectPoisonGeneratingRecipes(State);
----------------
Ayal wrote:
> OldInduction already set at State construction above?
>
> Also feed Vector/TripCounts to State constructor (indep. of this patch)?
removed in the latest version
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9108
+ DenseMap<Value *, VPValue *> ScalarIVs;
+ SmallVector<VPRecipeBase *> MoveAfterPhis;
for (BasicBlock *BB : make_range(DFS.beginRPO(), DFS.endRPO())) {
----------------
Ayal wrote:
> MoveAfterPhis and InductionsToMove suggest that buildVPlanWithVPRecipes() is trying to do too much too early.
> Both represent recipes that could not be accurately placed when constructed, and are later (re)positioned between last phi and first non-phi: InductionsToMove are trunc-folded-phi's that must first be placed at the position of the trunc before moving to appear among phi's, and MoveAfterPhis are VPExpandSCEVRecipes (that complement VPScalarIVStepsRecipes) which have yet to be placed - after all phi's but before other non-phi's (right? Some documentation would be helpful.)
>
> Could buildVPlanWithVPRecipes() be simplified by having
> - a Trunc(-that-can-be-folded-into-phi) recipe placed according to the Trunc, with a VPlan2VPlan transformation that traverses such Truncs and replaces them with a Phi recipe
> - a Phi(-that-can-be-converted-into-scalar-steps) recipe placed according to the Phi, with a VPlan2VPlan transformation that traverses such Phis and places them with pairs of VPScalarIVStepsRecipes and VPExpandSCEVRecipes
> - similar to WidenMemory recipes placed according to Load/Stores, later replaced by InterleaveGroup recipes
Those changes are gone now.
================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:176
+ VPScalarIVStepsRecipe *tryToBuildScalarSteps(Instruction *Instr,
+ Instruction *ScalarStepsForI,
+ VFRange &Range,
----------------
Ayal wrote:
> First parameter is always a Phi; second can be a potentially null Trunc, rather than Trunc-if-exists-otherwise-Phi?
This is gone in the latest version.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1287
+ auto *CanIV = cast<VPCanonicalIVPHIRecipe>(getOperand(0));
+ if (CanIV->getStartValue() != getOperand(1))
+ return false;
----------------
Ayal wrote:
> Add accessors instead of getOperand(0/1/2)?
>
> Comment that this checks if start value is zero, aka the start of the canonical IV.
Added accessors and comment, thanks.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:636
+ iplist<VPRecipeBase>::iterator I) {
assert(I == BB.end() || I->getParent() == &BB);
Parent = &BB;
----------------
Ayal wrote:
> assert Parent is null, as in the above insertBefore(InsertPos)?
> Place next to insertBefore(InsertPos) above, so that both moveAfter and moveBefore appear next to each other?
> There are in general 8 combinations: [ move | insert ][ After | Before ][ Recipe | {VPBB, I} ]
Done!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1095
+/// A recipe for handling phi nodes of integer and floating-point inductions,
+/// producing their vector and scalar values.
+class VPScalarIVStepsRecipe : public VPRecipeBase, public VPValue {
----------------
Ayal wrote:
> "their [vector and] scalar values"?
Removed, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1108
+ VPScalarIVStepsRecipe(PHINode *IV, const InductionDescriptor &IndDesc,
+ Instruction *Trunc, VPValue *CanonicalIV, VPValue *Step)
+ : VPRecipeBase(VPScalarIVStepsSC, {CanonicalIV, Step}),
----------------
Ayal wrote:
> Suffice to have a single constructor possibly with Trunc as its last optional parameter defaulted to null. Only distinction is the underlying value of VPValue which is set to (Trunc ? Trunc : IV).
> Separate pointer to Trunc is redundant, can instead check if the underlying value is a Trunc.
Simplified, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1119
+
+ /// Generate the vectorized and scalarized versions of the phi node as
+ /// needed by their users.
----------------
Ayal wrote:
> "[vectorized and]"
Removed, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:387
+ auto *IV = dyn_cast<VPWidenIntOrFpInductionRecipe>(&Phi);
+ if (!IV)
+ continue;
----------------
Ayal wrote:
> Have both early-exits here doing `if (!IV || IV->needsVectorIV()) continue;` ?
Moved, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:410
+ VPScalarIVStepsRecipe *Steps = new VPScalarIVStepsRecipe(
+ PhiI, ID, Plan.getCanonicalIV(), IV->getStartValue(), Step, TruncI);
+
----------------
Ayal wrote:
> nit: pass IV->getPHINode() and IV->getTruncInst() instead of PhiI and TruncI? This is essentially transforming the VPWidenIntOrFpInductionRecipe IV into a VPScalarIVStepsRecipe, along with a specific Step.
Done, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:415
+ HeaderVPBB->insert(cast<VPRecipeBase>(Step->getDef()),
+ HeaderVPBB->getFirstNonPhi());
+ IV->replaceAllUsesWith(Steps);
----------------
Ayal wrote:
> TODO: Step should be placed in preheader?
Added the TODO, thanks!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:59
+ /// Try to introduce a scalar-steps recipe for induction users only accessing
+ /// scalar elements.
+ static void optimizeInductions(VPlan &Plan, ScalarEvolution &SE);
----------------
Ayal wrote:
> If all users of vector IV need scalar values, provide them by building scalar steps off of the canonical scalar IV, and remove the vector IV.
Much better description, updated!
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:332
VPWidenSelectSC,
+ VPScalarIVStepsSC,
----------------
Ayal wrote:
> Lex order
Moved, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115953/new/
https://reviews.llvm.org/D115953
More information about the llvm-commits
mailing list