RFR: 8367627: C2: Missed Ideal() optimization opportunity with MemBar [v3]

[Benoît Maillard]

On Mon, 24 Nov 2025 09:56:10 GMT, Benoît Maillard <bmaillard at openjdk.org> wrote:

>> This PR addresses a missed optimization in `PhaseIterGVN` for `MemBarAcquire` nodes caused by a missing notification during parsing.
>> 
>> The missed optimization in question is the removal of the the `in(MemBarNode::Precedent)` edge for
>> `MemBarAcquire` nodes when the the `MemBar` is the only user of its input. This was intially
>> introduced to get rid of unused `Load` nodes that were only kept alive by such an edge.
>> 
>> 
>> 
>> https://github.com/openjdk/jdk/blob/eeb7c3f2e8e645938d9db0cf61c1d98d751f2845/src/hotspot/share/opto/memnode.cpp#L4254-L4259
>> 
>> In our case, it happens that the `Load` node gets folded to a constant during the initial
>> `_gvn.transform` call in `GraphKit::make_load`. Because the value is converted before being
>> returned, we end up with two constant nodes: one `ConL` and one `ConI`. The `ConL` only
>> has one usage, and this triggers the optimization during verification.
>> 
>> 
>> static int test0() {
>>     var c = new MyClass();
>>     // the conversion ensures that the ConL node only has one use
>>     // in the end, which triggers the optimization
>>     return (int) c.l;
>> }
>> 
>> 
>> The optimization is not triggered earlier during when we apply `_gvn.transform` on the membar,
>> because it requires `can_reshape`, which is set to `false` in when we call `apply_ideal` in
>> `PhaseGVN::transform`.
>> 
>> For this reason, we should call `record_for_igvn(membar)` after the `MemBar` is created
>> and transformed in `GraphKit::insert_mem_bar` to make sure it gets an `Ideal` pass with
>> `can_reshape` later.
>> 
>> 
>> This issue was initially filed for Valhalla, because a condition in `GraphKit::make_load`
>> prevents its from occurring when boxing elimination is enabled. Boxing elimination is
>> disabled temporarily in Valhalla (see [JDK-8328675](https://bugs.openjdk.org/browse/JDK-8328675)),
>> which caused the issue to appear, but by using `-XX:-EliminateAutoBox` it became clear
>> that the issue was on mainline.
>> 
>> ### Testing
>> - [x] [GitHub Actions](TODO)
>> - [x] tier1-3, plus some internal testing
>> 
>> Thank you for reviewing!
>
> Benoît Maillard 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-8367627
>  - Add notification in Node::has_special_unique_user
>  - Add run with -XX:+AlwaysIncrementalInline, and add intermediate run for -XX:-DoEscapeAnalysis
>  - Record in GraphKit::insert_mem_bar_volatile for consistency
>  - Improve test and fix
>  - Add test

The issue is fixed now. It seems that we also need to add the constant case in `Node::has_special_unique_user()`. Apparently `-XX:+AlwaysIncrementalInline` delays folding of the `Load` node, which causes the pattern to only appear later, and then we need a notification when the number of uses changes.

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

PR Comment: https://git.openjdk.org/jdk/pull/28448#issuecomment-3575135812