RFR: 8360031: C2 compilation asserts in MemBarNode::remove [v2]

[Damon Fenacci]

On Fri, 5 Sep 2025 09:45:38 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Damon Fenacci has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8360031
>>  - JDK-8360031: update assert message
>>  - Merge branch 'master' into JDK-8360031
>>  - JDK-8360031: remove unnecessary include
>>  - JDK-8360031: remove UseNewCode
>>  - JDK-8360031: compilation asserts in MemBarNode::remove
>
> I stepped through the crash with the replay file, and I'm not convinced that the problem is only with MemBarStoreStore and not MemBarRelease.  What happens in the replay crash is the MemBarStoreStore gets onto the worklist through an indirect route in ConnectionGraph::split_unique_types() because of its memory edge.  I think this explains why it is intermittent and hard to reproduce.  A MemBarRelease on the other hand would get added to the worklist directly in compute_escape() if it has a Precedent edge.
> The different handling of MemBarStoreStore vs MemBarRelease in this code is confusing.  The MemBarRelease code came from JDK-6934604.  It adds the node to the worklist, and lets MemBarNode::Ideal remove it based on does_not_escape_thread() on the alloc node.  Contrast that with the MemBarStoreStore handling, which came from JDK-7121140, and instead of removing the node, it replaces it with a MemBarCPUOrder based on not_global_escape() on the alloc node.  This MemBarStoreStore handling is for "MemBarStoreStore nodes added in library_call.cpp" and seems to fail to work for  MemBarStoreStore nodes added in the ctor, which means MemBarStoreStore nodes added in the ctor only get on the worklist by accident, as mentioned above.
> I think the conservative fix is to have compute_escape() always add the MemBarStoreStore to the worklist if it has a Precedent edge.  Because of StressIGVN randomizing the worklist, I think the outcnt() can be 1 for either MemBarStoreStore or MemBarRelease, so we should relax the assert accordingly.  I'm not sure how useful the assert will be after that.  It might be better to remove it.
> Longer-term, it might be nice to get rid of the separate handling of "MemBarStoreStore nodes added in library_call.cpp" if the MemBarCPUOrder is not really needed.

Thanks a lot for your reviews @dean-long @shipilev @vnkozlov!

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

PR Comment: https://git.openjdk.org/jdk/pull/26556#issuecomment-3384286660