[PATCH] D111301: [VPlan] Make sure recurrence splice is not inserted between phis.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

In D111301#3115048 <https://reviews.llvm.org/D111301#3115048>, @Ayal wrote:

> In D111301#3112774 <https://reviews.llvm.org/D111301#3112774>, @fhahn wrote:
>
>> In D111301#3109192 <https://reviews.llvm.org/D111301#3109192>, @Ayal wrote:
>>
>>> In D111301#3108536 <https://reviews.llvm.org/D111301#3108536>, @fhahn wrote:
>>>
>>>> ping :)
>>>>
>>>> @Ayal does the clarification why this is needed make sense?
>>>>
>>>>> (from inline comment)
>>>>> Ok I now what phi-like case I originally hit: if the previous value is an induction value! So I think we still need the special handling?
>>>
>>> Clarification indeed makes sense, induction feeding a FOR should presumably be handled, not being a 2nd-order recurrence (SOR).
>>>
>>> So we may indeed still need the special handling - along with special test(s); perhaps using i-1 inside a "for i" loop?
>>
>> There should be test coverage already for those cases, in `Transforms/LoopVectorize/induction.ll`, which fails verification from D111302 <https://reviews.llvm.org/D111302> without the current patch.
>
> ok; can a comment be added to these tests, mentioning that they exercise induction-feeding-a-FOR, and careful placement of the FOR splice?
> Wonder how VPlan generated correct code, with splice placed in the midst of phi's(?)

Done in the committed version. The issue in the test case were other induction recipes after the splice. The existing `widenIntOrFpInduction` inserts new phis at the beginning of the BB, so it did not cause any issues during actual codegen.

>>> Would also be good to address/exclude/cover non-header phis - of replicated regions and blends.
>>
>> Should those be included in this patch here?
>
> or left as a TODO?

I did not add a TODO in the committed version, because unfortunately I am not sure where to best add one :(



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9456
       continue;
 
+    VPRecipeBase *PrevRecipe = RecurPhi->getBackedgeRecipe();
----------------
Ayal wrote:
> nit: can tentatively set
> 
> ```
>   VPBasicBlock *InsertBlock = PrevRecipe->getParent();
> ```
> initially, and later override it with
> 
> ```
>   InsertBlock = cast<VPBasicBlock>(Region->getSingleSuccessor());
> ```
Done in the committed version, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111301