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

[Tobias Hartmann]

On Tue, 17 Nov 2020 09:09:59 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> 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;
> }

Thanks for the reviews!

@rwestrel Yes, exactly. I was thinking about such a refactoring as well but then decided to go with the simple fix because the code is guarded by `#ifdef ASSERT` anyway. Do you intent to integrate that refactoring with another change? In that case I would like to use the simple fix here.

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

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