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

[Aleksey Shipilev]

On Wed, 28 May 2025 21:46:37 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> But the whole point of this PR is that "current behavior" is incorrect, isn't it?
>> 
>> I think disabling `_Reference_reachabilityFence` intrinsic (or, failing to inline the intrinsic for some other reason) should fail-safe to non-inlined method, not fail-deadly to a broken RF. In other words, let's not rely on intrinsic to work for correctness; non-intrinsified version should be correct as well.
>> 
>> I agree `@DontInline` would require a bit of extra fiddling in C1, but I suspect it should be as easy as copy-pasting a few hunks around `LIRGenerator::do_blackhole`.
>
>> But the whole point of this PR is that "current behavior" is incorrect, isn't it?
> 
> Strictly speaking, current implementation has a defect and it requires a complete rewrite on C2 side to properly fix it.
> 
> Current implementation is part of JDK for a long time (since 11). It's highly unlikely it'll be backported all the way to JDK 11 and it's an open question whether it should be backported at all. So, for diagnostic purposes it makes sense to provide a way to compare old and new implementations irrespective of whether old implementation still has the bug.
> 
>> In other words, let's not rely on intrinsic to work for correctness; non-intrinsified version should be correct as well.
> 
> A question for you: do you think we should test non-intrinsified case?
> 
> Personally, I consider such requirement as way too strong. In this particular case, the method is unconditionally intrinsified in C2. If no intrinsification takes place, it's a bug. (I'm fine with adding an assert & abort compilation if C2 ever observes `Reference.reachabilityFence()` to be inlined.)

I think two general principles apply here: a) intrinsics are performance optimizations, not correctness building blocks; b) when _forced to choose_, we prefer correctness over performance.

This is not about the backports, but about having the correct fallback when we need it. Imagine, for a second, this fix has a major bug discovered later. We instruct users to disable the intrinsic to avoid that bug. Users then ask: "Is it safe to disable the intrinsic? Would my application crash/misbehave without it? Would it run slower?". Right now we have a choice what we can answer. With `@DontInline`, we say "Yes, it would run slower, but RF would still work against misbehaviors". Without `@DontInline`, we say "Yes, there is a possibility of misbehavior, but at least it would run as fast". One of these answers goes against the general principle (b).

I also cannot remember any existing intrinsic that is a counter-example for (a). Without `@DontInline`, RF would be a first?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2113331201