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

[Jatin Bhateja]

On Tue, 10 Oct 2023 08:50:32 GMT, Xiaohong Gong <xgong 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.

Marked as reviewed by jbhateja (Committer).

Thanks for the explanation, looks good to me.

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

PR Review: https://git.openjdk.org/valhalla/pull/937#pullrequestreview-1669776816
PR Comment: https://git.openjdk.org/valhalla/pull/937#issuecomment-1756708972