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

Sun Nov 6 16:49:47 PST 2022

fhahn added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9541
+  DerivedIV =
+      emitTransformedIndex(State.Builder, DerivedIV,
+                           getStartValue()->getLiveInIRValue(), Step, IndDesc);
----------------
Ayal wrote:
> 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?
> nit: can also rename this original emitTransformedIndex() - emitDerivedIndex()

Done!

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

It looks like this is only needed for the use here and not the other uses, so it seems simpler to keep to code here?

================
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)))
----------------
Ayal wrote:
> 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?
Thanks, updated to keep adding the step to VPScalarIVStepsRecipe as well.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9573
+  if (Step->getType() != BaseIV->getType())
+    Step = State.Builder.CreateTrunc(Step, BaseIV->getType());

----------------
Ayal wrote:
> 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.)
at the moment, a derived IV recipe is created if only a truncate is needed, but the derived IV will only be truncated at the end, so it uses the wide step whereas for the steps recipe we need the truncated step. 

I think to unify this we would have to compute the derived IV with truncated steps, but that would be a bigger, unrelated change,

================
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");
----------------
Ayal wrote:
> ... or DerivedIV ...
added, thanks

================
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())
----------------
Ayal wrote:
> Also check for same type, as claimed at the interface?
updated, thanks!

================
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();
+
----------------
Ayal wrote:
> nit: define `ID` and `TruncI` below slightly closer to first use?
done, thanks!

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:402
+    VPCanonicalIVPHIRecipe *CanIV = Plan.getCanonicalIV();
+    VPValue *IVR = CanIV;
+    Type *IVTy = IV->getPHINode()->getType();
----------------
Ayal wrote:
> `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`.
Updated, thanks! I kept the type as VPVale as this requires using getDef just once.

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:405
+    Type *TruncTy = TruncI ? TruncI->getType() : IVTy;
+    if (!CanIV->isCanonical(ID) || IVTy != TruncTy) {
+      VPValue *Step =
----------------
Ayal wrote:
> 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?
done, thanks!

================
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,
----------------
Ayal wrote:
> nit: (re)use `IVTy`
> nit: use `Can[onical]IV` directly instead of `IVR`.
done, thanks!

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:411
+                                  TruncI ? TruncI->getType() : nullptr);
+      HeaderVPBB->insert(cast<VPRecipeBase>(IVR->getDef()), IP);
+    }
----------------
Ayal wrote:
> nit: was there some helper to get Def as RecipeBase?
the patch has not landed yet (D136068)

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:102
     VPVWidenSelectSC,
+    VPVTransformedIVSC,

----------------
Ayal wrote:
> Transformed or Derived? Lex order
done thanks!

================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:356
     VPScalarIVStepsSC,
+    VPDerivedIVSC,
     VPWidenCallSC,
----------------
Ayal wrote:
> Lex order
done thanks!

================
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]]>
----------------
Ayal wrote:
> While we're here, `ir<false>`, `ir<true>` seem odd (and even ;-)) 
I guess that's because they are boolean values and the IR get printed as boolean literals by the IR printer. Do you think this is something that should be changed?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133758