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

[Roland Westrelin]

On Wed, 14 Oct 2020 10:01:59 GMT, Christian Hagedorn <chagedorn 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.

@chhagedorn @vnkozlov thanks for the review

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

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