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

[Tobias Hartmann]

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
>