[PATCH] D133758: [VPlan] Add VPTransformedIVRecipe, use for VPScalarIVStepsRecipe.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Nice refactoring - potentially closing a gap between VPlan and original post-vectorization IR sink scalar operands?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8679
   auto *StartV = Plan.getOrAddVPValue(StartIdx);
+  auto *StepV = Plan.getOrAddVPValue(ConstantInt::get(IdxTy, 1));
 
----------------
Can alternatively provide the desired constant 1 VPValue when asked to retrieve getStep()?
(Such constant operands are analogous to  Attributes in MLIR.)
(Unlike the constant Start which VPlan::prepareToExecute() may replace later with a nonzero value, something worth fixing...)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9511
 
+void VPTransformedIVRecipe::execute(VPTransformState &State) {
+  // Fast-math-flags propagate from the original induction instruction.
----------------
`assert(!State.Instance && "VPTransformedIVRecipe being replicated.");` ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9521
+  Value *ScalarIV = State.get(getCanonicalIV(), VPIteration(0, 0));
+  Value *TransformedIV =
+      Ty->isIntegerTy()
----------------
Some things seem a bit confusing here, looking at the existing code:

VPScalarIVStepsRecipe::getCanonicalIV() presumably retrieves the same Recipe/VPValue as does the enclosing VPlan->getCanonicalIV()?

The original code has both `ScalarIV` and `CanonicalIV` - are they not the same - one retrieves a Value per lane (0,0) and the other per part (0) - used only to check its type?

Now `TransformedIV` is also "Scalar" (as in non-Vector) similar to `ScalarIV`.

Perhaps instead of
`Value *ScalarIV = State.get(getCanonicalIV(), VPIteration(0, 0));`
we should have
`Value *CanonicalIV = State.get(getCanonicalIV(), VPIteration(0, 0));`
?

Perhaps instead of `TransformedIV` have `NonCanonicalIV`, `AffineIV` or `DerivedIV` - considering that the canonical IV is aka a "BasicIV"?

Then rename `VPTransformedIVRecipe` accordingly?

Would be good to explain somewhere all the IV recipes together: those representing a single scalar (canonical or not) across VF&UF, a single vector per part, a single scalar per lane.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9534
+    TransformedIV = State.Builder.CreateTrunc(TransformedIV, TruncToTy);
+  }
+
----------------
assert(TransformedIV != ScalarIV && "..."); ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9549
 
-  Value *Step = State.get(getStepValue(), VPIteration(0, 0));
-  auto CreateScalarIV = [&](Value *&Step) -> Value * {
-    Value *ScalarIV = State.get(getCanonicalIV(), VPIteration(0, 0));
-    auto *CanonicalIV = State.get(getParent()->getPlan()->getCanonicalIV(), 0);
-    if (!isCanonical() || CanonicalIV->getType() != Ty) {
-      ScalarIV =
-          Ty->isIntegerTy()
-              ? State.Builder.CreateSExtOrTrunc(ScalarIV, Ty)
-              : State.Builder.CreateCast(Instruction::SIToFP, ScalarIV, Ty);
-      ScalarIV = emitTransformedIndex(State.Builder, ScalarIV,
-                                      getStartValue()->getLiveInIRValue(), Step,
-                                      IndDesc);
-      ScalarIV->setName("offset.idx");
-    }
-    if (TruncToTy) {
-      assert(Step->getType()->isIntegerTy() &&
-             "Truncation requires an integer step");
-      ScalarIV = State.Builder.CreateTrunc(ScalarIV, TruncToTy);
-      Step = State.Builder.CreateTrunc(Step, TruncToTy);
-    }
-    return ScalarIV;
-  };
+  Value *ScalarIV = State.get(getOperand(0), VPIteration(0, 0));
+  VPValue *StepVPV;
----------------
ditto: everything here is scalar IV. Perhaps BaseIV, FirstLaneScalarIV, or Start - reviving getStartValue() to wrap getOperand(0)?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9552
+  if (auto *CanIV = dyn_cast<VPCanonicalIVPHIRecipe>(getOperand(0)))
+    StepVPV = CanIV->getStepValue();
+  else
----------------
Is this the source for introducing a unit step as operand to Can[onical]IV?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9554
+  else
+    StepVPV = cast<VPTransformedIVRecipe>(getOperand(0))->getStepValue();
+
----------------
Have both producing recipes feed VPScalarIVStepsRecipe() with their step value as another operand, reviving getStepValue() to retrieve it?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:642
   // When vectorizing the epilogue loop, the canonical induction start value
   // needs to be changed from zero to the value after the main vector loop.
   if (CanonicalIVStartValue) {
----------------
Suggest to add a FIXME to find a better way than this for VPlan to represent epilogue loop.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1131
 /// start value is the first operand of the recipe and the incoming value from
 /// the backedge is the second operand.
 class VPHeaderPHIRecipe : public VPRecipeBase, public VPValue {
----------------
second >> last


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1988
+
+  /// Generate the scalarized versions of the phi node as needed by their users.
+  void execute(VPTransformState &State) override;
----------------
Worth clarifying which scalarized versions are actually generated.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:405
+      // The start value of the steps-recipe must match the start value of the
+      // canonical induction and it must step by 1.
+      if (CanIV->getStartValue() != Start)
----------------
Should this be a method of VPCanonicalIVPHIRecipe, checking if a given Start and Step match those of its own?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:414
+    auto IP = HeaderVPBB->getFirstNonPhi();
+    VPValue *IVR = Plan.getCanonicalIV();
+    Type *IVTy = IV->getPHINode()->getType();
----------------
Suggest to first set `VPCanonicalIVPHIRecipe *CanonicalIV = Plan.getCanonicalIV();` (or auto) to avoid casting below. Then set VPValue *Start, Step to be either those of CanonicalIV or those of the new VPTransformedIVRecipe, to feed the new VPScalarIVStepsRecipe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133758