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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D113183#3153441 <https://reviews.llvm.org/D113183#3153441>, @fhahn wrote:

> Addressed comments and added a few additional code comments.
>
> In D113183#3153076 <https://reviews.llvm.org/D113183#3153076>, @Ayal wrote:
>
>> 
>
>
>
>> Yes, in order to sink LastInduction it should be wrapped in a recipe by itself, separating it from VPWidenIntOrFpInductionRecipe. Would introducing such a LastInduction recipe here, potentially created in FixHeaderPhis out of its feeding/consuming VPWidenIntOrFpInductionRecipe, be simpler for this patch? It needs only to take of
>
> I had a look, but unfortunately I don't think this is feasible additional work. The step increment value depends on the induction descriptor and may need to be expanded from a SCEV expression in the pre-header. Once we can model the preheader in VPlan, we can add a recipe in the preheader to create the step value.

OK.
There are actually two issues here: (1) sinking LastInduction to the latch, and (2) feeding LastInduction to the header phi as its operand associated with the latch.
Another temporary workaround may be have ILV::widenIntOrFpInduction() return the LastInduction it generates (in the header, w/o feeding it to the phi), and have VPWidenIntOrFpInductionRecipe record this LastInduction directly. Note that ILV::widenIntOrFpInduction() might not generate a LastInduction - when (unrolling w/o vectorizing) it does not call createVectorIntOrFpInductionPHI(). In such case VPWidenIntOrFpInductionRecipe should not be formed, right?

> Alternatively we could land D111303 <https://reviews.llvm.org/D111303> first, which expands the step up front and maps it to a VPValue that can be used by the induction recipes. Please let me know if you prefer that direction.
>
> A further complication is that LastInduction is also set for `CastDef`.
>
>>   LastInduction = cast<Instruction>(
>>       Builder.CreateBinOp(AddOp, LastInduction, SplatVF, "step.add"));
>>   LastInduction->setDebugLoc(EntryVal->getDebugLoc());
>>
>> keeping VPWidenIntOrFpInductionRecipe with a single value Def.



> Unfortunately VPWidenIntOrFpInductionRecipe is already a multi-def in some cases (due to handling casts as well).

wonder how R->addOperand(IncR->`getVPSingleValue()`); currently works in fixHeaderPhis - is a testcase of a casted induction feeding a reduction is needed?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1211
       // handled.
+      // There is also nothing to drop from VPWidenIntOrFpInductionRecipe.
       if (isa<VPWidenMemoryInstructionRecipe>(CurRec) ||
----------------
fhahn wrote:
> Ayal wrote:
> > Is this addition of excluding VPWidenIntOrFpInductionRecipe independent of this patch?
> I think it makes sense to exclude it in any case, but it is becoming problematic with the patch (due to adding additional defs, causing a crash in `getUndderlyingValue). I *think* it could also cause issues if the induction recipe has a CastDef.
Any other (header phi?) recipes worth excluding? (Independent of this patch)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2447
+  State.set(StepDef, LastInduction, 0);
+  State.set(PhiDef, VecInd, 0);
   VecInd->addIncoming(SteppedStart, LoopVectorPreHeader);
----------------
fhahn wrote:
> Ayal wrote:
> > `PhiDef` is redundant? `VecInd` is already being State.set above as part 0 of `Def`.
> Unfortunately `Def` may be reset to something other than the phi, e.g. in `CreateSplatIV `.
> Unfortunately Def may be reset to something other than the phi, e.g. in CreateSplatIV .
This truly deserves simplification. Or at-least some explanation what Def refers to in contrast to PhiDef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113183