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

[Antón Seoane Ampudia]

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

Hi! Wanted to mention this might be related to the following: [JDK-8330062](https://bugs.openjdk.org/browse/JDK-8330062), which I'm giving a look at the moment

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

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