[PATCH] D115953: [VPlan] Introduce recipe to build scalar steps.

Mon Dec 27 07:41:46 PST 2021

Ayal added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8426
+      ++NewInsertionPoint;
+    Builder.setInsertPoint(Builder.getInsertBlock(), NewInsertionPoint);
+
----------------
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.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8934
+
+  if (onlyScalarStepsNeeded(ScalarStepsForI, Range)) {
+    const SCEV *StepSCEV = II->getStep();
----------------
early-exit if !OnlyScalarStepsNeeded()

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8943
+    else
+      Step = new VPExpandSCEVRecipe(II->getStep(), *PSE.getSE());
+
----------------
Pass StepSCEV instead of II->getSteps()?

This VPExpandSCEVRecipe needs to be placed somewhere by every caller of tryToBuildScalarSteps.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8948
+    return new VPScalarIVStepsRecipe(Phi, *II, ScalarStepsForI,
+                                     Plan.getCanonicalIV(), Step);
+  }
----------------
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.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9115
+  DenseMap<Value *, VPValue *> VectorIVs;
+  DenseMap<Value *, VPValue *> ScalarIVs;
+  SmallVector<VPRecipeBase *> MoveAfterPhis;
----------------
VectorIVs and ScalarIVs seem useless?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9145
+      if (auto *Steps =
+              RecipeBuilder.tryToBuildScalarSteps(Instr, Instr, Range, *Plan)) {
+        Plan->addVPValue(Instr, Steps);
----------------
Rather than pass Instr twice, pass it once as the "Phi" and null as the potential Trunc?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9151
+                dyn_cast_or_null<VPRecipeBase>(Steps->getOperand(1)->getDef()))
+          MoveAfterPhis.push_back(StepR);
+        continue;
----------------
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.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9174

+        RecipeBuilder.setRecipe(Instr, Recipe);
         if (isa<VPWidenIntOrFpInductionRecipe>(Recipe) &&
----------------
Why is this setRecipe moved?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9190
+          if (auto *StepR = dyn_cast_or_null<VPRecipeBase>(Step->getDef()))
+            MoveAfterPhis.push_back(StepR);
+          continue;
----------------
(This records both VPExpandSCEVRecipe and its VPScalarIVStepsRecipe created by tryToCreateWidenRecipe() for Trunc Instr., to be placed later.)

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9815
+void VPScalarIVStepsRecipe::execute(VPTransformState &State) {
+  Value *Step = State.get(getOperand(1), VPIteration(0, 0));
+  auto CreateScalarIV = [&](Value *&Step) -> Value * {
----------------
Store per-part 0?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9817
+  auto CreateScalarIV = [&](Value *&Step) -> Value * {
+    Value *ScalarIV = State.get(getOperand(0), VPIteration(0, 0));
+    if (IV != State.OldInduction) {
----------------
ditto

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10246
   if (!hasScalarValue(Def, {Part, LastLane})) {
     // At the moment, VPWidenIntOrFpInductionRecipes can also be uniform.
+    assert((isa<VPWidenIntOrFpInductionRecipe>(Def->getDef()) ||
----------------
Update comment

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:560
+  /// The induction variable of the old basic block.
+  PHINode *OldInduction = nullptr;
+
----------------
OldInduction moved from protected to public (in some version?) - suffice to provide a const public method to retrieve OldInduction?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2776
+  assert(!shouldScalarizeInstruction(EntryVal));
+  createVectorIntOrFpInductionPHI(ID, Step, Start, EntryVal, Def, State);
   Value *ScalarIV = CreateScalarIV(Step);
----------------
Can be folded with above call to createVectorIntOrFpInductionPHI(), followed by

```
  if (NeedsScalarIV) {
    Value *ScalarIV = CreateScalarIV(Step);
    buildScalarSteps(ScalarIV, Step, EntryVal, ID, Def, State);
  }
```
?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7935
   State.VectorTripCount = ILV.getOrCreateVectorTripCount(nullptr);
+  State.OldInduction = ILV.OldInduction;
   ILV.collectPoisonGeneratingRecipes(State);
----------------
OldInduction already set at State construction above?

Also feed Vector/TripCounts to State constructor (indep. of this patch)?

================
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())) {
----------------
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

================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:174
       VPlanPtr &Plan);

+  VPScalarIVStepsRecipe *tryToBuildScalarSteps(Instruction *Instr,
----------------
Document?

================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:176
+  VPScalarIVStepsRecipe *tryToBuildScalarSteps(Instruction *Instr,
+                                               Instruction *ScalarStepsForI,
+                                               VFRange &Range,
----------------
First parameter is always a Phi; second can be a potentially null Trunc, rather than Trunc-if-exists-otherwise-Phi?

================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:636
+                                iplist<VPRecipeBase>::iterator I) {
   assert(I == BB.end() || I->getParent() == &BB);
   Parent = &BB;
----------------
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} ]

================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:721
   void insertBefore(VPRecipeBase *InsertPos);
+  void insertBefore(VPBasicBlock &BB, iplist<VPRecipeBase>::iterator I);

----------------
(Here the insertBefores appear next to each other.)

================
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 {
----------------
"their [vector and] scalar values"?

================
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}),
----------------
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.

================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1119
+
+  /// Generate the vectorized and scalarized versions of the phi node as
+  /// needed by their users.
----------------
"[vectorized and]"

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:332
     VPWidenSelectSC,
+    VPScalarIVStepsSC,

----------------
Lex order

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115953/new/

https://reviews.llvm.org/D115953