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

[Damon Fenacci]

On Tue, 9 Sep 2025 15:37:48 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 incrementally with one additional commit since the last revision:
> 
>   JDK-8360031: add assert condition and make consume method argument escape

The fix made the `ConstructorBarrier.java` JTREG test fail because the argument of the `consume` method wasn't actually escaping (and IGVN was removing the MemBar). So I added an assignment to a volatile field to make it escape.

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

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