[PATCH] D113183: [LV] Patch up induction phis after VPlan execution.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2413
+  LastIncrementOfVectorIV->setName("vec.ind.next");
   VecInd->addIncoming(SteppedStart, LoopVectorPreHeader);
+  return LastIncrementOfVectorIV;
----------------
Add a comment that the other incoming will be added later?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2652
       auto *Add = Builder.CreateBinOp(AddOp, SplatIV, Mul);
       State.set(Def, Add, Part);
       // It's useful to record the lane values too for the known minimum number
----------------
This State.set() seems to be the culprit.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:833
   // Fix the latch value of reduction and first-order recurrences phis in the
   // vector loop.
   VPBasicBlock *Header = Entry->getEntryBasicBlock();
----------------
Augment above comment to also include induction phis?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:854
+      auto *LoopVectorLatch =
+          State->LI->getLoopFor(State->CFG.PrevBB)->getLoopLatch();
+      auto *Br = cast<BranchInst>(LoopVectorLatch->getTerminator());
----------------
LoopVectorLatch aka VectorLatchBB?
Rewiring of all header phi's should ideally look similar, as done below for reductions and FORs. Inductions should use their LastIncrement as Val, and are also SinglePartNeeded.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:864
     if (!PhiR || !(isa<VPFirstOrderRecurrencePHIRecipe>(&R) ||
                    isa<VPReductionPHIRecipe>(&R)))
       continue;
----------------
Keep this early-exit if !PhiR || !(FOR || Reduction || [Induction]) as the first thing in the loop?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:849
+        Cur = cast<Instruction>(Cur->getOperand(0));
+      PHINode *VecInd = cast<PHINode>(Cur);
+      // Move the last step to the end of the latch block. This ensures
----------------
fhahn wrote:
> Ayal wrote:
> > Can VecInd be found instead by looking up State.get(IV..), as done for reductions and FOR rewired below?
> Unfortunately that isn't possible, because `State.get(IV,...)` may not refer to the phi directly but may be reset to something other than the phi, e.g. in `CreateSplatIV `
`State.get(IV->getVPSingleValue(), 0)` does seem to provide the desired header phi, for **non-scalable** vectors. However, this State may indeed be reset for **scalable** vectors. Better to try and fix problems caused by resetting State, than to bypass them.

(CreateSplatIV() sets the State for IV rather than resets it, and only for scalar IV's. Here we're dealing with vector IV's).
This resetting stems from buildScalarSteps() in D98715, which resets the State for both scalar and vector IV's. The latter case resets per-part vectors only if they are scalable (and phi is not UAV). But if only a vector IV is needed, it is built w/o calling buildScalarSteps(), i.e., w/o resetting the State for IV. Why does build*Scalar*Steps() need to reset per-part vectors, only when scalar users are present?

If still needed, the actual PhiOfVectorIV could also be recorded alongside LastIncrementOfVectorIV, instead of tracking it down (or up). This could be done using a separate (derived) induction recipe for scalable vectors. In general, if multiple IR values associated with the same recipe need to be retrieved later to hook-up users and/or definitions, they could be modelled using a multi-VPValued VPDef.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1015
+  /// modeling the increment  explicitly in VPlan.
+  Instruction *LastInduction = nullptr;
+
----------------
fhahn wrote:
> Ayal wrote:
> > Better rename `LastInduction` to "LastIncrementOfVectorIV", throughout?
> > 
> > Scalar IV's are generated w/o a cross-iteration increment, based off of `Induction`, which is pre-generated as part of the skeleton, along with its last increment in the latch.
> > 
> > An alternative of forming a Region with an Exit to represent the latch, is mentioned in D114586.
> > Better rename LastInduction to "LastIncrementOfVectorIV", throughout?
> 
> Done, thanks!
> 
> > An alternative of forming a Region with an Exit to represent the latch, is mentioned in D114586.
> 
> I'll follow up on the suggestion, but I don't see how this is related directly to this patch. Future patches will remove the need to keeping the last induction increment here by modeling the increment explicitly in VPlan. But to support the general induction case, we need to support expand the step for the induction from a SCEV expression in the pre-header. I'll put patches up to do this once we can model the pre-header.
>> An alternative of forming a Region with an Exit to represent the latch, is mentioned in D114586.
> ... how this is related directly to this patch
This alternative may have two (alternative?) parts:
Having the Exit VPBasicBlock available when creating header phi recipes will enable placing last increments there to begin with, instead of moving them (or their generated IR Instruction). This requires introducing recipes/VPInstructions to model these increments, as proposed in follow-up D113223, so indeed more directly related to that patch.
Having the IR Latch BasicBlock available when header phi recipes execute will enable them to generate their last increments and rewire them there and then, w/o needing to record and rewire later. This requires generating the IR basic block of the latch along with that of the header, presumably by VPRegionBase::execute(). But instead of having induction header-phi  recipes be responsible for generating their entire chain of increments, it may be better if they behave like reduction and FOR header-phi recipes, whose 'previous' or last reduction operations have independent recipes; this supports a simple linear code-generation of VPlan, instead of moving a builder's insert position around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113183