[PATCH] D113223: [VPlan] Add VPCanonicalIVRecipe, partly retire createInductionVariable.

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 27 13:12:16 PST 2021


fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3008
 
   IRBuilder<> B(&*Header->getFirstInsertionPt());
   Instruction *OldInst = getDebugLocFromInstOrOperands(OldInduction);
----------------
Ayal wrote:
> Initialize B to Latch->getTerminator(), instead of setting it there below after initializing it to first insertion point of Header?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4531
+  auto *IVR = Plan->getCanonicalIV();
+  PHINode *CanonicalIV = cast<PHINode>(State.get(IVR, VPIteration(0, 0)));
+
----------------
Ayal wrote:
> `State.get(IVR, 0)`, per-part 0?
Adjusted!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7936
+  // When vectorizing the epilogue loop, the canonical induction start value
+  // needs to be changed to the value after the main vector loop.
+  if (CanonicalIVStartValue) {
----------------
Ayal wrote:
> "... changed [from zero] to ..."
Thanks, I added the extra part.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8946
+
+  VPBasicBlock *EB = TopRegion->getExit()->getExitBasicBlock();
+  if (IsVPlanNative)
----------------
Ayal wrote:
> Can do `TopRegion->getExitBasicBlock()` directly, similar to TopRegion->getEntryBasicBlock() above.
Thanks, adjusted!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1308
+void VPCanonicalIVPHIRecipe::execute(VPTransformState &State) {
+  Value *Start = getOperand(0)->getLiveInIRValue();
+  PHINode *EntryPart = PHINode::Create(
----------------
Ayal wrote:
> getOperand(0) >> getStartValue()?
updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:933
+                       ? State->get(R.getVPSingleValue(), VPIteration(Part, 0))
+                       : State->get(PhiR, Part);
+      Value *Val =
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Storing per-part 0 rather than per-lane 0 and common base class should lead to a common State->get(PhiR, Part) for all, so the loop becomes
> > > 
> > > ```
> > >     for (unsigned Part = 0; Part < NumPartsForNewPhi; ++Part) {
> > >       Value *Phi = State->get(PhiR, Part);
> > >       Value *Val = State->get(BackedgeValue, IsSinglePart ? SinglePart : Part);
> > >       cast<PHINode>(Phi)->addIncoming(Val, VectorLatchBB);
> > >     }
> > > ```
> > Storing a scalar value in the 'vector' part of State seems not ideal, but it does indeed simplify things a lot here. Updated
> That is how uniform (/UAV) scalar values are stored in general, namely, in the 'vector' part of State.
> The scalar canonical IV is conceptually UAV, only its lane 0 is used.
One issue with that representation is that it is not possible to get a vector with the scalar splatted I think. Not an issue at the moment here though.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:824
 void VPlan::execute(VPTransformState *State) {
   // -1. Check if the backedge taken count is needed, and if so build it.
   if (BackedgeTakenCount && BackedgeTakenCount->getNumUsers()) {
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Should all invariant values that are generated in the pre-header be modelled as VPValues similar to BackedgeTakenCount, until the preheader is modelled as VPBB with recipes? I.e., treat VectorTripCount, [Canonical]IVStartValue and "CanonicalIVStepValue" (returned by createStepForVF()) similar to VPlan's BackedgeTakenCount, which is hooked-up to its State->TripCount IR Value here?
> > I am not sure if it is worth it at the moment, as long as they are only used by a single recipe. Specifically, the start value is mostly a constant (0), the step is currently only used by the induction increment and the vector trip count is currently only used in post-processing. VectorTripCount is moved to a dedicated VPValue in the patch that introduces recipes for the exit condition. The step value could be moved out as follow up. WDYT? 
> > I am not sure if it is worth it at the moment, as long as they are only used by a single recipe. Specifically, the start value is mostly a constant (0), the step is currently only used by the induction increment and the vector trip count is currently only used in post-processing. VectorTripCount is moved to a dedicated VPValue in the patch that introduces recipes for the exit condition. The step value could be moved out as follow up. WDYT?
> 
> Perhaps instead of passing everything via one VPTransformState into VPlan::execute(), it would be better to separate and first have VPlan initialize all its live-in/pre-header VPValues, and then dedicate VPTransformState to hold only general context required during code-gen?
That's a good point! I put up D116320 and then added a VPValue for the VectorTripCount in this patch


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1099
            V->getVPValueID() == VPValue::VPVReductionPHISC;
   }
 
----------------
Ayal wrote:
> Keep above in Lex order?
moved `WidenPHI` to the back.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2297
+
+  VPCanonicalIVRecipe *getCanonicalIV() {
+    VPBasicBlock *EntryVPBB = getEntry()->getEntryBasicBlock();
----------------
Ayal wrote:
> Ayal wrote:
> > Start by calling getVectorLoopRegion()?
> > 
> > Associate getCanonicalIV() with a (non replicating) Region, instead of 'entire' VPlan?
> > Cache a pointer to VPCanonicalIVRecipe in Region?
> > (IV's of replicating regions are eliminated by complete unrolling.)
> Can add to VPlanVerifier a check that first recipe only is VPCanonicalIVPHIRecipe?
Sounds good, added!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113223



More information about the llvm-commits mailing list