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

[Tobias Hartmann]

On Wed, 19 Apr 2023 08:11:54 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
>
> Eric Liu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge jdk:master
>    
>    Change-Id: I63c06b87d5b0c20ddaec0aa43031872b8ebb5362
>  - fix typo
>    
>    Change-Id: I1b84c4957398178bf234f71242a1cdd044181a79
>  - 8304948: [vectorapi] C2 crashes when expanding VectorBox
>    
>    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#L2557
>    [2] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/cfgnode.cpp#L2574
>    [3] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/cfgnode.cpp#L2534
>    [4] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/vector.cpp#L316
>    
>    Change-Id: I381b1ba7e0865814d97535e365db6d9d72ef1949

Another review would be good.

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

PR Comment: https://git.openjdk.org/jdk/pull/13489#issuecomment-1514321737