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

[Eric Liu]

On Tue, 18 Apr 2023 07:49:56 GMT, Tobias Hartmann <thartmann 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
>
> 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?

I think it is.

The CYCLE is derived from merge_through_phi, in which only Phi CYCLE is allowed [1].

This method `expand_vbox_node_helper` serves to locate the target node `Proj(VectorBoxAllocate)` and then replace it with a punch of other nodes. When expanding VectorBox, the normal case is `VectorBox (Proj value)`. If it is in shape of `VectorBox (Phi1 Phi2)`[2] or `VectorBox (Phi1 vect)`[3],  I suppose it should be transformed by merge_through_phi.

In `expand_vbox_node_helper`, it is recursive only at Phi1 since it is where Proj in. 

[1] https://github.com/e1iu/jdk/blob/ENTLLT-4778-external/src/hotspot/share/opto/cfgnode.cpp#L2574
[2] https://github.com/e1iu/jdk/blob/ENTLLT-4778-external/src/hotspot/share/opto/vector.cpp#L331
[3] https://github.com/e1iu/jdk/blob/ENTLLT-4778-external/src/hotspot/share/opto/vector.cpp#L341

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

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