RFR: 8254734: "dead loop detected" assert failure with patch from 8223051

[Christian Hagedorn]

On Wed, 14 Oct 2020 07:52:58 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> compiler/c2/TestDeadDataLoopIGVN.java ran with -server -Xcomp
> -XX:+CreateCoredumpOnCrash -ea -esa -XX:CompileThreshold=100
> -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation and
> the patch from 8223051 (long counted loops, not integrated yet) fails
> with:
> 
> assert(no_dead_loop) failed: dead loop detected
> 
> I can reproduce the failure with only this change from 8223051:
> diff --git a/src/hotspot/share/opto/callnode.cpp b/src/hotspot/share/opto/callnode.cpp
> index 268cec7732c..295cb4ecaf9 100644
> --- a/src/hotspot/share/opto/callnode.cpp
> +++ b/src/hotspot/share/opto/callnode.cpp
> @@ -1159,7 +1159,7 @@ Node* SafePointNode::Identity(PhaseGVN* phase) {
>    if( in(TypeFunc::Control)->is_SafePoint() )
>      return in(TypeFunc::Control);
>  
> -  if( in(0)->is_Proj() ) {
> +  if (in(0)->is_Proj() && !phase->C->major_progress()) {
>      Node *n0 = in(0)->in(0);
>      // Check if he is a call projection (except Leaf Call)
>      if( n0->is_Catch() ) {
> diff --git a/src/hotspot/share/opto/parse1.cpp b/src/hotspot/share/opto/parse1.cpp
> index 4b88c379dea..baf2bf9bacc 100644
> --- a/src/hotspot/share/opto/parse1.cpp
> +++ b/src/hotspot/share/opto/parse1.cpp
> @@ -2254,23 +2254,7 @@ void Parse::return_current(Node* value) {
>  
>  //------------------------------add_safepoint----------------------------------
>  void Parse::add_safepoint() {
> -  // See if we can avoid this safepoint.  No need for a SafePoint immediately
> -  // after a Call (except Leaf Call) or another SafePoint.
> -  Node *proj = control();
>    uint parms = TypeFunc::Parms+1;
> -  if( proj->is_Proj() ) {
> -    Node *n0 = proj->in(0);
> -    if( n0->is_Catch() ) {
> -      n0 = n0->in(0)->in(0);
> -      assert( n0->is_Call(), "expect a call here" );
> -    }
> -    if( n0->is_Call() ) {
> -      if( n0->as_Call()->guaranteed_safepoint() )
> -        return;
> -    } else if( n0->is_SafePoint() && n0->req() >= parms ) {
> -      return;
> -    }
> -  }
>  
>    // Clear out dead values from the debug info.
>    kill_dead_locals();
> so it's unrelated to the long counted loops transformation itself.
> 
> At IGVN time, a dead loop is optimized. The loop contains the
> following subgraph:
> 
> (LoadI .. (AddP (CastPP .. (Phi (Proj (CallStaticJava .. (AddI
> 
> The projection is on the backedge of the phi. The AddI points back to
> the LoadI. The CallStaticJava is a boxing method call. The LoadI is a
> boxed value load.
> 
> The phi is optimized out as the loop is unreachable:
> 
> (LoadI .. (AddP (Proj (CallStaticJava .. (AddI
> 
> The LoadI is then optimized by code MemNode::can_see_stored_value():
>     // Load boxed value from result of valueOf() call is input parameter.
>     if (this->is_Load() && ld_adr->is_AddP() &&
>         (tp != NULL) && tp->is_ptr_to_boxed_value()) {
>       intptr_t ignore = 0;
>       Node* base = AddPNode::Ideal_base_and_offset(ld_adr, phase, ignore);
>       BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
>       base = bs->step_over_gc_barrier(base);
>       if (base != NULL && base->is_Proj() &&
>           base->as_Proj()->_con == TypeFunc::Parms &&
>           base->in(0)->is_CallStaticJava() &&
>           base->in(0)->as_CallStaticJava()->is_boxing_method()) {
>         return base->in(0)->in(TypeFunc::Parms);
>       }
>     }
> to the AddI. Because the AddI has the LoadI as input we end up with a
> dead loop. I propose extending the dead loop safe logic to take the
> pattern with a boxing method load into account.

That's a reasonable fix. I was thinking about something similar before when I was fixing JDK-8251544. Looks good to me.

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/649