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

[Eric Liu]

On Wed, 19 Apr 2023 19:08:57 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> 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
>
> src/hotspot/share/opto/vector.cpp line 323:
> 
>> 321:   if (visited.test_set(vbox->_idx)) {
>> 322:     assert(vbox->is_Phi(), "not a phi");
>> 323:     return vbox; // already visited
> 
> Shouldn't the short circuit return a transformed node instead? Or it does not matter here? Thanks.

It does not matter. It will finally be transformed in the round, in which it is the root. At this round, it's an input of another Phi.

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

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