[lworld+vector] RFR: 8317699: [lworld+vector] Fix Vector API tests crash with "assert(vbox->is_Phi()) failed: should be phi"

[Xiaohong Gong]

On Wed, 11 Oct 2023 03:29:02 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Currently several jtreg tests under `test/jdk/jdk/jdk/incubator/vector` crash with following error:
>> 
>> 
>> A fatal error has been detected by the Java Runtime Environment:
>> 
>> Internal Error (valhalla/src/hotspot/share/opto/vector.cpp:257), pid=2737779, tid=2737796
>> assert(false) failed: should be phi
>> 
>> 
>> It happens when expanding a `VectorBoxNode` which has following inputs:
>> 
>>                         VectorBoxAllocate
>>                                |
>>                                |
>>        VectorBoxAllocate    [2 Proj]
>>                |             /   |
>>             [1 Proj]       /     |
>>                \         /       |
>>                  \     /        /
>>                    \ /        /
>>                  [2 Phi]    /
>>                      \    /
>>                        \ |
>>                      [1 Phi]     InlineType
>>                          \      /
>>                            \  /
>>                          VectorBox
>> 
>> The compiler will visit and expand all the inputs of the `PhiNode`, and mark them visited. If the input has been visited, it just returns without any action. And the compiler assumes the revisited node should be a PhiNode. In above graph, the first `Phi` (i.e. `[1 Phi]`) has the same input `[2 Proj]` with the second `Phi` (i.e. `[2 Phi]`). So `[2 Proj]` will be visited twice. At the second time it is visited, the assertion fails since it is not a Phi. The case is reasonable in java and the assertion can be removed.
>> 
>> Removing the assertion can fix the existing issue and no new issues are involved.
>> 
>> Jdk mainline also has the same issue, but I prefer fixing it here temporarily with following reasons:
>>  1) Several jtreg tests crash on this branch while pass on jdk mainline. This blocks our testing and further development.
>>  2) We can have more testing with this fix on this branch. And port it to jdk mainline once we think it's a mature/safe change.
>
> Thanks for the explanation, looks good to me.

Thanks for the review @jatin-bhateja !

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

PR Comment: https://git.openjdk.org/valhalla/pull/937#issuecomment-1756709707