[PATCH] D133758: [VPlan] Add VPDerivedIVRecipe, use for VPScalarIVStepsRecipe.
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Oct 23 08:19:55 PDT 2022
Ayal added a comment.
Ahh, this brings up some further thoughts...
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9541
+ DerivedIV =
+ emitTransformedIndex(State.Builder, DerivedIV,
+ getStartValue()->getLiveInIRValue(), Step, IndDesc);
----------------
nit: can also rename this original emitTransformedIndex() - emitDerivedIndex()
TODO: have emitTransformedIndex() also take care of casting its 2nd "Index" parameter to Ty instead of asserting it's the same as that of its 4th "Step" parameter?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9565
+ Value *BaseIV = State.get(getOperand(0), VPIteration(0, 0));
+ VPValue *StepVPV;
+ if (auto *CanIV = dyn_cast<VPCanonicalIVPHIRecipe>(getOperand(0)))
----------------
Ahh, operand 0 of VPScalarIVStepsRecipe is providing both the start value (directly - BaseIV) and the step (indirectly - by casting operand 0 into a recipe and asking it for its step). Better represent all def-use relations explicitly by passing VPValues (only) between recipes.
This can be done by propagating Step instead of delegating it, e.g.: if SCEV is needed then have a common VPExpandSCEVRecipe (placed in the preheader) take care of generating the `VPValue *Step = vputils::getOrCreateVPValueForSCEVExpr(Plan, ID.getStep(), SE);` and feed it to both VPDerivedIVRecipe and VPScalarIVStepsRecipe. If SCEV is not used, have a Plan.getOrAddVPValue(1)) of the desired type feed these recipes?
Sounds reasonable?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9573
+ if (Step->getType() != BaseIV->getType())
+ Step = State.Builder.CreateTrunc(Step, BaseIV->getType());
----------------
This truncation of Step is needed only if fed directly from CanonicalIV, because the Step produced by DeriveIV should have the desired (value and) type, including a truncation at the end if needed, right?
Alternatively, introduce a DeriveIV recipe also if only truncation of Step is needed, so as not to sink it into triangles? (Or have ScalarIV recipe take care of TruncToTy always, also for derived steps, relieving DeriveIV of doing so, though this would sink into triangles.)
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:659
"the canonical IV should only be used by its increments or "
"ScalarIVSteps when "
"resetting the start value");
----------------
... or DerivedIV ...
================
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(1); }
----------------
nit: inlining can be applied separately.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1860
/// Returns the scalar type of the induction.
- const Type *getScalarType() const {
+ Type *getScalarType() const {
return getOperand(0)->getLiveInIRValue()->getType();
----------------
Check if this method can remain const if this recipe no longer needs to delegate its Step.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1865
+ /// Returns a VPValue for the step (constant 1) of the induction.
+ VPValue *getStepValue();
+
----------------
Consider removing and feeding users directly with a constant 1 VPValue when needed upon their construction, to keep this recipe with a single value (Start) VPDef. Otherwise it can provide both Start and Step as parts of its multi-valued VPDef...
(nit: const?)
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1875
+ /// Check if the induction described by \p ID is canonical, i.e. has a step of
+ /// 1 and the same type and start as the canonical IV.
+ bool isCanonical(const InductionDescriptor &ID) const;
----------------
nit: extra space "and the"
step is also the same (1), as type and start; or does the original canonical unit step become UF * VF?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1961
/// Scalar type to use for the generated values.
Type *Ty;
/// If not nullptr, truncate the generated values to TruncToTy.
----------------
Worth clarifying the use of both Ty (type to cast to before conversion) and TruncToTy (type to cast to after conversion)?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1996
+ /// Generate the transformed value of the induction at offset StartValue (2.
+ /// operand) + IV (1. operand) * StepValue (3, operand).
+ void execute(VPTransformState &State) override;
----------------
nit: 2nd, 1st, 3rd? Also clarify involved type casts?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2005
+
+ VPCanonicalIVPHIRecipe *getCanonicalIV() const;
+ VPValue *getStartValue() const { return getOperand(1); }
----------------
Return operand 0 inline, or outlined to avoid cast?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1055
+ // The start value of the steps-recipe must match the start value of the
+ // canonical induction and it must step by 1.
+ if (getStartValue()->getLiveInIRValue() != ID.getStartValue())
----------------
Also check for same type, as claimed at the interface?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:398
const InductionDescriptor &ID = IV->getInductionDescriptor();
- VPValue *Step =
- vputils::getOrCreateVPValueForSCEVExpr(Plan, ID.getStep(), SE);
Instruction *TruncI = IV->getTruncInst();
+
----------------
nit: define `ID` and `TruncI` below slightly closer to first use?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:402
+ VPCanonicalIVPHIRecipe *CanIV = Plan.getCanonicalIV();
+ VPValue *IVR = CanIV;
+ Type *IVTy = IV->getPHINode()->getType();
----------------
`IVR` may be a confusing name, defined as a VPValue* rather than a Recipe. `IV` also stands for a recipe, conflicting with `IVR`.
How about renaming `IVR` to something like `BaseIV`, and define it as a VPRecipeBase*? It stands for the recipe providing a single scalar value per iteration of vectorized & unrolled loop with the desired type/start/step values, as a Base on which to build scalar steps - a scalar value per lane and part. This is either the canonical IV recipe if suitable or a newly introduced derived IV recipe which transforms it.
Perhaps also rename `IV` to `WidenIV`, and spell out `CanIV` to `CanonicalV`.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:405
+ Type *TruncTy = TruncI ? TruncI->getType() : IVTy;
+ if (!CanIV->isCanonical(ID) || IVTy != TruncTy) {
+ VPValue *Step =
----------------
TruncTy here is the desired type of the resulting scalar steps; should it be supplied to isCanonical() along with ID in order to check if CanonicalIV is providing the suitable start/step/type for scalar users of `Phi`, or else a derived recipe is needed?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:408
+ vputils::getOrCreateVPValueForSCEVExpr(Plan, ID.getStep(), SE);
+ IVR = new VPDerivedIVRecipe(IV->getPHINode()->getType(), ID, IVR,
+ IV->getStartValue(), Step,
----------------
nit: (re)use `IVTy`
nit: use `Can[onical]IV` directly instead of `IVR`.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:411
+ TruncI ? TruncI->getType() : nullptr);
+ HeaderVPBB->insert(cast<VPRecipeBase>(IVR->getDef()), IP);
+ }
----------------
nit: was there some helper to get Def as RecipeBase?
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:102
VPVWidenSelectSC,
+ VPVTransformedIVSC,
----------------
Transformed or Derived? Lex order
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:356
VPScalarIVStepsSC,
+ VPDerivedIVSC,
VPWidenCallSC,
----------------
Lex order
================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-tail-folding-forced.ll:22
; VPLANS-NEXT: ACTIVE-LANE-MASK-PHI vp<%5> = phi vp<%3>, vp<%10>
-; VPLANS-NEXT: vp<%6> = SCALAR-STEPS vp<%4>, ir<0>, ir<1>
+; VPLANS-NEXT: vp<%6> = SCALAR-STEPS vp<%4>
; VPLANS-NEXT: CLONE ir<%gep> = getelementptr ir<%ptr>, vp<%6>
----------------
This does appear less descriptive than having SCALAR-STEPS depict the step it uses?
================
Comment at: llvm/test/Transforms/LoopVectorize/interleave-and-scalarize-only.ll:73
; DBG-NEXT: EMIT vp<[[CAN_IV:%.+]]> = CANONICAL-INDUCTION
-; DBG-NEXT: vp<[[STEPS1:%.+]]> = SCALAR-STEPS vp<[[CAN_IV]]>, ir<false>, ir<true>
-; DBG-NEXT: vp<[[STEPS2:%.+]]> = SCALAR-STEPS vp<[[CAN_IV]]>, ir<0>, ir<1>
+; DBG-NEXT: vp<[[DERIVED_IV:%.+]]> = DERIVED-IV vp<[[CAN_IV]]>, ir<false>, ir<true>
+; DBG-NEXT: vp<[[STEPS1:%.+]]> = SCALAR-STEPS vp<[[DERIVED_IV]]>
----------------
While we're here, `ir<false>`, `ir<true>` seem odd (and even ;-))
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