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

[Dean Long]

On Thu, 14 Aug 2025 10:54:08 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> # Issue
>> While compiling `java.util.zip.ZipFile` in C2 this assert is triggered
>> https://github.com/openjdk/jdk/blob/a2e86ff3c56209a14c6e9730781eecd12c81d170/src/hotspot/share/opto/memnode.cpp#L4235
>> 
>> # Cause
>> While compiling the constructor of java.util.zip.ZipFile$CleanableResource the following happens:
>> * we insert a trailing `MemBarStoreStore` in the constructor
>> <img height="200" alt="before_folding" src="https://github.com/user-attachments/assets/c1aab634-808d-4198-94ac-8093c6b85c5d" />
>> 
>> * during IGVN we completely fold the memory subtree of the `MemBarStoreStore` node. The node still has a control output attached. 
>> <img height="200" alt="after_folding" src="https://github.com/user-attachments/assets/568e9fc3-5f19-4e10-a72e-f0a5e772daed" />
>> 
>> * later during the same IGVN run the `MemBarStoreStore` node is handled and we try to remove it (because the `Allocate` node of the `MembBar` is not escaping the thread ) https://github.com/openjdk/jdk/blob/7b7136b4eca15693cfcd46ae63d644efc8a88d2c/src/hotspot/share/opto/memnode.cpp#L4301-L4302
>> * the assert https://github.com/openjdk/jdk/blob/7b7136b4eca15693cfcd46ae63d644efc8a88d2c/src/hotspot/share/opto/memnode.cpp#L4235
>> triggers because the barrier has only 1 (control) output and is a `MemBarStoreStore` (not `Initialize`) barrier
>> 
>> The issue happens only when the `UseStoreStoreForCtor` is set (default as well), which makes C2 use `MemBarStoreStore` instead of `MemBarRelease` at the end of constructors. `MemBarStoreStore` are processed separately by EA and this happens after the IGVN pass that folds the memory subtree. `MemBarRelease` on the other hand are handled during same IGVN pass before the memory subtree gets removed and it’s still got 2 outputs (assert skipped).
>> 
>> # Fix
>> Adapting the assert to accept that `MemBarStoreStore` can also have `!= 2` outputs (when `+UseStoreStoreForCtor` is used) seems to be an OK solution as this seems like a perfectly plausible situation.
>> 
>> # Testing
>> Unfortunately reproducing the issue with a simple regression test has proven very hard. The test seems to rely on very peculiar profiling and IGVN worklist sequence. JBS replay compilation passes. Running JCK's `api/java_util` 100 times triggers the assert a couple of times on average before the fix, none after.
>> Tier 1-3+ tests passed.
>
> 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.

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

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