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

[Eric Liu]

On Fri, 21 Apr 2023 18:20:27 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>>> But that phi will have an incorrect input, because the return value of this call is used as an input of the transformed phi that uses this node?
>> 
>> the return value of this call is Phi1. Phi1 is used as an input of Phi2 which is used by Phi1 as well. 
>> 
>> The Phi cycle is not an incorrect shape, it's a normal case generated by some simple cases, e.g., I have a test case in this patch.
>> When expanding VectorBox node, the purpose is to traverse the first input of VectorBox to locate Proj, and replace Proj with some other nodes.  The first input of VectorBox can be a graph, contains Phi (maybe Phi cycle) and Proj.
>> 
>> The process finding and replacing Proj is not in local graph, it creates a new graph at the same time. Return this visited node here is used to maintain that cycle. Besides Proj, nodes in graph should not be changed.
>
> 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.

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.

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

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