RFR: 8304948: [vectorapi] C2 crashes when expanding VectorBox [v2]

[Jatin Bhateja]

On Tue, 25 Apr 2023 16:15:18 GMT, Eric Liu <eliu at openjdk.org> wrote:

>>> Hi @e1iu , Can you please elaborate the reason for CYCLIC Phi creation in this case. 
>> 
>> To explain this issue, I also added a test case with the IR graph. The cycled Phi, I think is normal in C2 especially when it's two tiered loop. 
>> 
>> The two Phi nodes represent the value of `a` after inner loop and outer loop respectively. After outer loop, `a` should be a Phi(Phi1), one value is `a0` which is the initial value, another is the value after inner loop. The value of `a` after inner loop should also be a Phi(Phi2), one value is the initial  `a1`, another is the value  after outer loop, which is  Phi1.
>> 
>> In Vector API, `a0` and `a1` can be VectorBox.
>> 
>>> Ideally we should not have seen that pallet in first place. 
>> 
>> Cycled Phi has already been taken into consideration in Phi optimization. Please refer to the links [2] and [3]. If we disabled this shape in merge_thorugh_phi, I think the bug will be gone in this place. But that would loss some chances matching VectorBox and VectorUnbox.
>
>> I do still not fully grasp it though. When `expand_vbox_helper` is called on `Phi1`, it creates `NewPhi1`, then it calls `expand_vbox_helper` on `Phi2`, a `NewPhi2` is created, then `expand_vbox_helper` is invoked again on `Phi1`, which it shortcircuits to return `Phi1`, which will be attached as an input of `NewPhi2`, at this step should the second invocation on `Phi1` returns `NewPhi1` instead?
>> 
>> Thanks a lot.
> 
> Hi @merykitty , the fix is done. I think we dont need to create a new graph at all. Searching in local and just to replace the aimed Proj is more clear.

> > Hi @e1iu , Can you please elaborate the reason for CYCLIC Phi creation in this case.
> 
> To explain this issue, I also added a test case with the IR graph. The cycled Phi, I think is normal in C2 especially when it's two tiered loop.
> 
> The two Phi nodes represent the value of `a` after inner loop and outer loop respectively. After outer loop, `a` should be a Phi(Phi1), one value is `a0` which is the initial value, another is the value after inner loop. The value of `a` after inner loop should also be a Phi(Phi2), one value is the initial `a1`, another is the value after outer loop, which is Phi1.
> 
> In Vector API, `a0` and `a1` can be VectorBox.
> 
> > Ideally we should not have seen that pallet in first place.
> 
> Cycled Phi has already been taken into consideration in Phi optimization. Please refer to the links [2] and [3]. If we disabled this shape in merge_thorugh_phi, I think the bug will be gone in this place. But that would loss some chances matching VectorBox and VectorUnbox.

Thanks for the explanation, yes it makes sense now. During _merge_through_phi_  actual boxes are forwarded across the phi nodes and VBA are rewired to mimic original phi pattern, during box expansion we expand allocations in all the converging paths and because of cyclic pattern we enter into endless recusion resulting into stack overflow. I tired getting rid of VBA through early buffering but it leads to other set of [issues](https://github.com/openjdk/valhalla/pull/833#discussion_r1166386677)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13489#discussion_r1176841376