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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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);
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8530
 
-VPWidenIntOrFpInductionRecipe *VPRecipeBuilder::tryToOptimizeInductionTruncate(
+VPRecipeBase *VPRecipeBuilder::tryToOptimizeInductionTruncate(
     TruncInst *I, ArrayRef<VPValue *> Operands, VFRange &Range,
----------------
Is this change still needed or can tryToOptimizeInductionTruncate() continue to return VPWidenIntOrFpInductionRecipe?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9715
 
+void VPScalarIVStepsRecipe::execute(VPTransformState &State) {
+  // Fast-math-flags propagate from the original induction instruction.
----------------
This is to run once, i.e., not per part/lane inside a replicating region - assert(!State.Instance && "..."); as above.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1287
+  auto *CanIV = cast<VPCanonicalIVPHIRecipe>(getOperand(0));
+  if (CanIV->getStartValue() != getOperand(1))
+    return false;
----------------
Add accessors instead of getOperand(0/1/2)?

Comment that this checks if start value is zero, aka the start of the canonical IV.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:703
   void insertBefore(VPRecipeBase *InsertPos);
+  void insertBefore(VPBasicBlock &BB, iplist<VPRecipeBase>::iterator I);
 
----------------
Deserves a separate/additional comment?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1395
+/// A recipe for handling phi nodes of integer and floating-point inductions,
+/// producing their scalar values.
+class VPScalarIVStepsRecipe : public VPRecipeBase, public VPValue {
----------------
\p CanonicalIV produces a canonical scalar value per part, this recipe complements it by producing the possibly skewed scalar values per lane.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:387
+    auto *IV = dyn_cast<VPWidenIntOrFpInductionRecipe>(&Phi);
+    if (!IV)
+      continue;
----------------
Have both early-exits here doing `if (!IV || IV->needsVectorIV()) continue;` ?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:410
+    VPScalarIVStepsRecipe *Steps = new VPScalarIVStepsRecipe(
+        PhiI, ID, Plan.getCanonicalIV(), IV->getStartValue(), Step, TruncI);
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:415
+      HeaderVPBB->insert(cast<VPRecipeBase>(Step->getDef()),
+                         HeaderVPBB->getFirstNonPhi());
+    IV->replaceAllUsesWith(Steps);
----------------
TODO: Step should be placed in preheader?


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115953