RFR: 8256385: C2: fatal error: modified node is not on IGVN._worklist

[Roland Westrelin]

On Tue, 17 Nov 2020 09:00:50 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> `PhaseIdealLoop::find_safepoint` creates a temporary MergeMemNode that is not removed if we bail out from the optimization early (see `return NULL` statements). The fix is to simply add the MergeMem to the worklist to make sure it is always reclaimed by IGVN.
>> 
>> Interestingly this code path was not triggered by any of our tests but only with a test case generated by the Java Fuzzer. I've added a simplified version of that test case.
>> 
>> Thanks,
>> Tobias
>
> Looks good to me.

There's a call to _igvn.remove_dead_node(mm) below but the problem is that it's not always reached. I hit a similar problem while reworking the long counted loop support and fixed it by refactoring the code to:

    
  MergeMemNode* mm = NULL;
#ifdef ASSERT
    if (mem->is_MergeMem()) {
      mm = mem->clone()->as_MergeMem();
      for (MergeMemStream mms(mem->as_MergeMem()); mms.next_non_empty(); ) {
        if (mms.alias_idx() != Compile::AliasIdxBot && loop != get_loop(ctrl_or_self(mms.memory()))) {
          mm->set_memory_at(mms.alias_idx(), mem->as_MergeMem()->base_memory());
        }
      }
    }
#endif
    if (!no_side_effect_since_safepoint(C, x, mem, mm)) {
      safepoint = NULL;
    } else {
      assert(mm == NULL|| _igvn.transform(mm) == mem->as_MergeMem()->base_memory(), "all memory state should have been processed");
    }
#ifdef ASSERT
    if (mm != NULL) {
      _igvn.remove_dead_node(mm);
    }
#endif
with a new method:
static bool no_side_effect_since_safepoint(Compile* C, Node* x, Node* mem, MergeMemNode* mm) {
  SafePointNode* safepoint = NULL;
  for (DUIterator_Fast imax, i = x->fast_outs(imax); i < imax; i++) {
    Node* u = x->fast_out(i);
    if (u->is_Phi() && u->bottom_type() == Type::MEMORY) {
      Node* m = u->in(LoopNode::LoopBackControl);
      if (u->adr_type() == TypePtr::BOTTOM) {
        if (m->is_MergeMem() && mem->is_MergeMem()) {
          if (m != mem DEBUG_ONLY(|| true)) {
            for (MergeMemStream mms(m->as_MergeMem(), mem->as_MergeMem()); mms.next_non_empty2(); ) {
              if (!mms.is_empty()) {
                if (mms.memory() != mms.memory2()) {
                  return false;
                }
#ifdef ASSERT
                if (mms.alias_idx() != Compile::AliasIdxBot) {
                  mm->set_memory_at(mms.alias_idx(), mem->as_MergeMem()->base_memory());
                }
#endif
              } 
            }
          }
        } else if (mem->is_MergeMem()) {
          if (m != mem->as_MergeMem()->base_memory()) {
            return false;
          }
        } else {
          return false;
        }
      } else {
        if (mem->is_MergeMem()) {
          if (m != mem->as_MergeMem()->memory_at(C->get_alias_index(u->adr_type()))) {
            return false;
          }
#ifdef ASSERT
          mm->set_memory_at(C->get_alias_index(u->adr_type()), mem->as_MergeMem()->base_memory());
#endif
        } else {
          if (m != mem) {
            return false;
          }
        }
      }
    }
  }
  return true;
}

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

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