[PATCH] D133758: [VPlan] Add VPDerivedIVRecipe, use for VPScalarIVStepsRecipe.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 18 11:21:04 PDT 2022
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
More information about the llvm-commits
mailing list