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

[Tobias Hartmann]

On Mon, 17 Apr 2023 08:43:28 GMT, Eric Liu <eliu at openjdk.org> wrote:

> This patch fixes C2 failure with SIGSEGV due to endless recursion.
> 
> With test case VectorBoxExpandTest.java in this patch, C2 would generate IR graph like below:
> 
> 
>                ------------
>               /            \
>       Region |  VectorBox   |
>            \ | /            |
>             Phi             |
>              |              |
>              |              |
>       Region |  VectorBox   |
>            \ | /            |
>             Phi             |
>              |              |
>              |------------/
>              |
> 
> 
> 
> This Phi will be optimized by merge_through_phi [1], which transforms `Phi (VectorBox VectorBox)` into `VectorBox (Phi Phi)` to pursue opportunity of combining VectorBox with VectorUnbox. In this process, either the pre type check [2] or the process cloning Phi nodes [3], the circle case is well considered to avoid falling into endless loop.
> 
> After merge_through_phi, each input Phi of new VectorBox has the same shape with original root Phi before merging (only VectorBox has been replaced). After several other optimizations, C2 would expand VectorBox [4] on a graph like below:
> 
> 
>                ------------
>               /            \
>       Region |  Proj        |
>            \ | /            |
>             Phi             |
>              |              |
>              |              |
>       Region |  Proj        |
>            \ | /            |
>             Phi             |
>              |              |
>              |------------/
>              |
>              |     Phi
>              |     /
>           VectorBox
> 
> 
> which the circle case should be taken into consideration as well.
> 
> [TEST]
> Full Jtreg passed without new failure.
> 
> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/cfgnode.cpp#L2554
> [2] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/cfgnode.cpp#L2571
> [3] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/cfgnode.cpp#L2531
> [4] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/vector.cpp#L311

Looks reasonable to me.

src/hotspot/share/opto/vector.cpp line 307:

> 305: void PhaseVector::expand_vbox_node(VectorBoxNode* vec_box) {
> 306:   if (vec_box->outcnt() > 0) {
> 307:     VectorSet visited;

Do we need a ResourceMark here or above?

src/hotspot/share/opto/vector.cpp line 327:

> 325:   // Phi (VectorBox VectorBox) => VectorBox (Phi Phi)
> 326:   if (visited.test_set(vbox->_idx)) {
> 327:     assert(vbox->is_Phi(), "not a phi");

Are we sure that the cycle is always detected at a phi?

test/hotspot/jtreg/compiler/vectorapi/VectorBoxExpandTest.java line 83:

> 81:     //           VectorBox
> 82:     //
> 83:     // which the circle case should be taken into consideration as well.

Suggestion:

    // where the circle case should be taken into consideration as well.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13489#pullrequestreview-1389432514
PR Review Comment: https://git.openjdk.org/jdk/pull/13489#discussion_r1169593574
PR Review Comment: https://git.openjdk.org/jdk/pull/13489#discussion_r1169634345
PR Review Comment: https://git.openjdk.org/jdk/pull/13489#discussion_r1169591421