[PATCH] D121617: [LV] Move code to place induction increment to VPlan post-processing.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9533
+  // Add induction update using an incorrect block temporarily. The phi node
+  // will be fixed after VPlan execution.
+  VecInd->addIncoming(LastInduction, State.CFG.VectorPreHeader);
----------------
Ayal wrote:
> This goes back to D22416.
> Simply adding this last step here with its current block, as done prior to D22416, may be incorrect - when header and latch are distinct due to replicated regions? (May be good to (have a) test)
> 
> Would be good to explain that the last induction step, which feeds PHI, cannot be added yet to PHI properly, because that requires the latch block - which may not have been formed yet. An alternative to an incorrect addIncoming() may be to record LastInduction temporarily inside its VPWidenIntOrFpInductionRecipe (as it's operand(?) - see below), until it can be added to PHI properly. Given that LastInduction is eventually placed in the latch, another alternative may be to designate a recipe for generating LastInduction, placing it in the latch, so that when the recipe executes it could add LastInduction properly to PHI. Listing these alternatives mainly for the sake of completeness...
> This goes back to D22416.
> Simply adding this last step here with its current block, as done prior to D22416, may be incorrect - when header and latch are distinct due to replicated  regions? (May be good to (have a) test)

I think there are 2 distinct things going on. 1) moving the induction to the latch block and 2) adding the incoming value using the latch block. We could stop doing 1), without impacting correctness, but 2) is needed to generate valid IR in the presence of replicate regions. This should be handled by multiple existing tests.

> Would be good to explain that the last induction step, which feeds PHI, cannot be added yet to PHI properly, because that requires the latch block - which may not have been formed yet.

Done!

>  An alternative to an incorrect addIncoming() may be to record LastInduction temporarily inside its VPWidenIntOrFpInductionRecipe (as it's operand(?) - see below), until it can be added to PHI properly

Yes, but I think we would need to introduce a new VPValue for the increment, so we can map it properly. Adding the PHI as operand to itself wouldn't work I think, because in State we can only store values for each part, but we need the value of the last part + VF. Alternatively we could record the IR value in the recipe. But both of those seem like similar workarounds, but seems slightly more complicated. Another thing to note is that the CFG/IR LV generates during codegen is already temporarily invalid.


>  Given that LastInduction is eventually placed in the latch, another alternative may be to designate a recipe for generating LastInduction, placing it in the latch, so that when the recipe executes it could add LastInduction properly to PHI. Listing these alternatives mainly for the sake of completeness...

Agreed, but it would add extra clutter to the plan and also the code needed. Please let me know if any of the alternative is strictly preferable to the current approach.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:990
 
+    // Set the correct incoming block for backedge values and move induction to
+    // latch.
----------------
Ayal wrote:
> Could the incoming value to an induction be added similar to adding such values for reductions and FORs below, which have distinct values across the backedge? I.e., another case of SinglePartNeeded? Could PhiR->getBackedgeValue() work - if the induction PhiR is added as an (second) operand of itself?
> 
> Moving the last induction step to the end of the latch could then be done separately.
> Could the incoming value to an induction be added similar to adding such values for reductions and FORs below, which have distinct values across the backedge? I.e., another case of SinglePartNeeded? Could PhiR->getBackedgeValue() work - if the induction PhiR is added as an (second) operand of itself?

Unfortunately I don't think so, without further changes. The recipe for the phi is currently mapped to the vector IV values for each part. The increment is the last part + VF. So we either would have to turn VPWidenIntOrFpInductionRecipe into a  multi-def or store the LLVM IR value in the recipe directly. Both seem like bigger workarounds in my opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121617