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

[Eric Liu]

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

>> Yes, I think it should return the `NewPhi1` instead. 
>> 
>> In my test case, the `NewPhi1` and `NewPhi2` are idealized to `Phi1` and `Phi2`, so it does not matter whether it returns the new one. But I'm not sure if it's certain to be idealize to the old one. Anyway, return the new is more reasonable. I will fix that and do test.
>
>> 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.

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

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