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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8679
   auto *StartV = Plan.getOrAddVPValue(StartIdx);
+  auto *StepV = Plan.getOrAddVPValue(ConstantInt::get(IdxTy, 1));
 
----------------
Ayal wrote:
> 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...)
Updated, thanks!


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9521
+  Value *ScalarIV = State.get(getCanonicalIV(), VPIteration(0, 0));
+  Value *TransformedIV =
+      Ty->isIntegerTy()
----------------
Ayal wrote:
> 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.
> VPScalarIVStepsRecipe::getCanonicalIV() presumably retrieves the same Recipe/VPValue as does the enclosing VPlan->getCanonicalIV()?

Yes we could also use `VPlan->getCanonicalIV()`, but it might be easier to follow if modeled explicitly?

> 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?


Yep, that should be cleaner in the new code.

> Perhaps instead of

Thanks, I updated the naming to use `CanonicalIV` and `DerivedIV`. I also renamed `VPTransformedIVRecipe` -> `VPDerivedIVRecipe`

> 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.

Good idea, I'll see about that separately.


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


================
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;
----------------
Ayal wrote:
> ditto: everything here is scalar IV. Perhaps BaseIV, FirstLaneScalarIV, or Start - reviving getStartValue() to wrap getOperand(0)?
Updated to use `BaseIV`. I kept `getOperand(0)` for now, as it seems like it may be confused with the different behavior of the existing `getStartValue()`.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9554
+  else
+    StepVPV = cast<VPTransformedIVRecipe>(getOperand(0))->getStepValue();
+
----------------
Ayal wrote:
> Have both producing recipes feed VPScalarIVStepsRecipe() with their step value as another operand, reviving getStepValue() to retrieve it?
I left things as they are for now, after refactoring `getSTepValue` in `VPCanonicalIVPHIrecipe`.


================
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) {
----------------
Ayal wrote:
> Suggest to add a FIXME to find a better way than this for VPlan to represent epilogue loop.
Added, thanks!


================
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 {
----------------
Ayal wrote:
> second >> last
That's not needed any longer.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1172
   /// Returns the incoming value from the loop backedge.
-  VPValue *getBackedgeValue() {
-    return getOperand(1);
-  }
+  VPValue *getBackedgeValue() { return getOperand(getNumOperands() - 1); }
 
----------------
Should be gone now.


================
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;
----------------
Ayal wrote:
> Worth clarifying which scalarized versions are actually generated.
Added an explanation, thanks!


================
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)
----------------
Ayal wrote:
> Should this be a method of VPCanonicalIVPHIRecipe, checking if a given Start and Step match those of its own?
Added a helper.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:414
+    auto IP = HeaderVPBB->getFirstNonPhi();
+    VPValue *IVR = Plan.getCanonicalIV();
+    Type *IVTy = IV->getPHINode()->getType();
----------------
Ayal wrote:
> 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?
Updated to have a separate `CanIV` variable, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133758