RFR: 8290892: C2: Intrinsify Reference.reachabilityFence [v8]

[Vladimir Ivanov]

On Mon, 8 Sep 2025 12:45:56 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/callGenerator.cpp line 623:
>> 
>>> 621:       return; // keep the original call node as the holder of reachability info
>>> 622:     }
>>> 623:   }
>> 
>> Maybe that's just me. But people use the assert messages both in positive and negative ways, and so this is a bit ambiguous. Maybe you can write:
>> `no reachability edge should be present`
>> 
>> I'm still a bit unsure what the `SafePointNode::grow_stack` comment means.
>> In the previous comment https://github.com/openjdk/jdk/pull/25315#discussion_r2320120466 you explained more. Why not add that here instead?
>
> I'm also not sure yet why there is a difference between incremental inlining and regular inlining.
> Do you think it would make sense to explain that here, or is it explained elsewhere?

There are no safepoint-attached reachability edges present during normal parsing. For incremental inlining, JVMS from the original call is taken and extended with callee state. If there are reachability edges present, they have to be treated specially and carried over to all safepoints produced during incremental inlining attempt. There's no such support in place yet.

>> src/hotspot/share/opto/macro.cpp line 973:
>> 
>>> 971:         _igvn._worklist.push(ac);
>>> 972:       } else if (use->is_ReachabilityFence() && OptimizeReachabilityFences) {
>>> 973:         use->as_ReachabilityFence()->clear_referent(_igvn); // redundant fence
>> 
>> Thanks for refactoring a bit here :)
>> 
>> Is this rf guaranteed to belong to the Allocation somehow?
>
> Ah, you could mention that later `ReachabilityFenceNode::Identity` removes the rf.

> Is this rf guaranteed to belong to the Allocation somehow?

I don't get your question. The code iterates over users of an allocation which is being eliminated.  Semantically, RF is a no-op on a scalarizable referent and has to be removed in order to let the scalarization happen.

> Ah, you could mention that later ReachabilityFenceNode::Identity removes the rf.

Done.

>> src/hotspot/share/opto/reachability.cpp line 136:
>> 
>>> 134:     return true;
>>> 135:   }
>>> 136: }
>> 
>> Nit: `an no-op` -> `a no-op`
>> 
>> Also: do you need the return value? The only use case does not do anything with it.
>
> You could mention that `Identity` will remove the node later.

> Also: do you need the return value? The only use case does not do anything with it.

I decided to keep it for diagnostic purposes even though no existing callers care about it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2334899185
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2337978454
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2337989028