[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