RFR(S): 8244407: JVM crashes after transformation in C2 IdealLoopTree::split_fall_in

[Vladimir Kozlov]

Hi, Felix

Yes, the fix looks good.

Please, use {} for if() body.

Thanks,
Vladimir

On 5/11/20 1:58 AM, Tobias Hartmann wrote:
> Hi Felix,
> 
> your fix looks reasonable to me but Vladimir K. (who reviewed your original fix), should also have a
> look.
> 
> Best regards,
> Tobias
> 
> On 06.05.20 03:32, Yangfei (Felix) wrote:
>> Hi,
>>
>>    Please help review this patch fixing a C2 crash issue.
>>    Bug: https://bugs.openjdk.java.net/browse/JDK-8244407
>>    Webrev: http://cr.openjdk.java.net/~fyang/8244407/webrev.00
>>
>>    After the fix for JDK-8240576, irreducible loop tree might be structurally changed by split_fall_in() in IdealLoopTree::beautify_loops.
>>    But loop tree is not rebuilt after that. Take the reported test case for example, irreducible loop tree looks like:
>>
>> 1: Loop: N649/N644
>>
>> Loop: N649/N644  IRREDUCIBLE    <-- this
>>    Loop: N649/N797  sfpts={ 683 }
>>
>> With the fix for JDK-8240576，we won't do merge_many_backedges in IdealLoopTree::beautify_loops for this irreducible loop tree.
>>
>>    if( _head->req() > 3 && !_irreducible) {
>>      // Merge the many backedges into a single backedge but leave
>>      // the hottest backedge as separate edge for the following peel.
>>      merge_many_backedges( phase );
>>      result = true;
>>    }
>>
>> N660                 N644         N797
>>    |                             |             |
>>    |                             |             |
>>    |                             v             |
>>    |             +---+---+             |
>>    +-----> +  N649  + <-----+
>>                   +--------+
>>
>>   649    Region  ===  649  660  797  644  [[ .... ]]  !jvms: Test::testMethod @ bci:543
>>
>> Then we come to the children:
>>
>>     // Now recursively beautify nested loops
>>     if( _child ) result |= _child->beautify_loops( phase );
>>
>> 2: Loop: N649/N797
>>
>>   Loop: N649/N644  IRREDUCIBLE
>>    Loop: N649/N797  sfpts={ 683 }    <-- this
>>
>> After spilt_fall_in()，N660 and N644 are merged.
>>
>>      if( fall_in_cnt > 1 )         // Need a loop landing pad to merge fall-ins
>>        split_fall_in( phase, fall_in_cnt );
>>
>> N660                               N644
>>    |                                          +
>>    |                                          |
>>    |                                          |
>>    |          +---------+            |
>>    +---->+  N946   +<-----+
>>                +----+---+
>>               |                                      N797
>>               |                                          |
>>               |                                          |
>>               |                                          |
>>               |           +--------+            |
>>               +----> +  N649  + <-----+
>>                            +--------+
>>
>> Loop tree is now structurally changed into:
>>
>>    Loop: N946/N644  IRREDUCIBLE
>>      Loop: N649/N797  sfpts={ 683 }
>>
>> But local variable 'result' in IdealLoopTree::beautify_loops hasn't got a chance to be set to true since _head->req() is not bigger than 3 after split_fall_in.
>> Then C2 won't rebuild loop tree after IdealLoopTree::beautify_loops, which further leads to the crash.
>> Instead of adding extra checking for loop tree structure changes, proposed fix sets 'result' to true when we meet irreducible loop with multiple backedges.
>> This should be safer and simpler (thus good for JIT compile time).
>> Tiered 1-3 tested on x86-64 and aarch64 linux platform.  Comments?
>>
>> Thanks,
>> Felix
>>