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

Thu May 22 22:44:12 UTC 2025

On Thu, 22 May 2025 09:15:03 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   review feedback
>
> src/hotspot/share/opto/block.cpp line 189:
> 
>> 187:          !get_node(end_idx)->is_Mach() &&
>> 188:          !get_node(end_idx)->is_BoxLock() &&
>> 189:          !(get_node(end_idx)->is_ReachabilityFence() && C->print_assembly())) {
> 
> Um. So this fairly generic method is predicated of whether a diagnostic VM option is enabled. Which risks that the compiler behavior with/without printing assembly is different? Which might hide the very issues we are trying to diagnose with printing assembly? Can we handle `RF` here without checking for `print_assembly`?
> 
> This will also obviate a need to pass `Compile* C` around.

Fair enough. Treating RFs uniformly should be fine, since they are attached to safepoints now.

> src/hotspot/share/opto/callnode.cpp line 969:
> 
>> 967:             projs->exobj = e;
>> 968:           } else  {
>> 969:             // exception table for rethrow case
> 
> Feels like we want to assert other values of `e->in(0)->as_CatchProj()->_con` here? From the switch statement in the previous hunk, there seems to be `fall_through_index` that is not "rethrow case" (or is it?).

Makes sense. Added an assert.

> src/hotspot/share/opto/compile.cpp line 3912:
> 
>> 3910: // requires that the walk visits a node's inputs before visiting the node.
>> 3911: 
>> 3912: static bool has_non_debug_uses(Node* n) {
> 
> This got inserted right between  `------------------------------final_graph_reshaping_walk--------------------` comment and the `final_graph_reshaping_walk` implementation. Also, put `has_non_debug_uses` into `Compile`?

I moved it to `Node::has_non_debug_uses()`.

> src/hotspot/share/opto/compile.cpp line 3995:
> 
>> 3993:     for (int j = start; j < end; j++) {
>> 3994:       Node* in = n->in(j);
>> 3995:       if (in->is_DecodeNarrowPtr() && (is_uncommon || has_non_debug_uses(in))) {
> 
> The comment says we can skip when node is only referenced in debug info. Here we skip when there _are_ non-debug uses. Did you mean `!has_non_debug_uses(in)`?

Good point. Fixed.

Funnily enough, it did the right job here (but not in RF case!), because `has_non_debug_uses` reported the opposite answer to what can be expected based on its name :-)

> src/hotspot/share/opto/parse1.cpp line 1225:
> 
>> 1223: 
>> 1224:   if (StressReachabilityFences) {
>> 1225:     // Keep all oop arguments alive until method return.
> 
> Comment says "arguments", but we save locals. Aren't arguments on "stack" in `JVMState`? For stress mode, would make sense to hook up both locals/stack from `JVMState`, maybe?

It happens inside callee context, so all arguments are already moved to locals. The code could explicitly iterate over arguments (using `argument(uint)` query) or enumerate only those locals which hold arguments, but that would require a special case for receiver. Iteration over locals (`[0 ... max_locals)`) is uniform and enumerates only arguments since everything else is top.

> src/hotspot/share/opto/reachability.cpp line 211:
> 
>> 209:     }
>> 210:   }
>> 211:   return found;
> 
> `found` is always `false` here. I don't think this function even needs a return value, judging by the uses.

My intention was to signal when not-yet-known redundant fences are found. 
I prefer to keep it even if it is not used right now.
Fixed by updating `found` along with pushing newly discovered redundant node on the list.

> src/hotspot/share/opto/reachability.cpp line 431:
> 
>> 429:       }
>> 430:     }
>> 431:     redundant_rfs.push(rf);
> 
> I see `PhaseIdealLoop::optimize_reachability_fences` asks for `redundant_rfs.member(rf)` before going into this analysis. Is it because we can have duplicate RFs in the `C->reachability_fence` list? Can we have duplicate here? Should we check `.member(rf)` here as well?

No duplicated entries are allowed in `Compile::_reachability_fences`.

`eliminate_reachability_fences()` is called at the end of loop opts after `optimize_reachability_fences()` is done (possibly, multiple times). 

`eliminate_reachability_fences()` migrates all referents from RFs to safepoints. So, every RF node is placed on `redundant_rfs` list and there are no RF nodes left after `eliminate_reachability_fences()` is over (there's `C->reachability_fences_count() == 0` assert at the end).

After safepoints are directly linked to the referent, the corresponding RF node becomes redundant. So, I kept `redundant_rfs` as a name. I can choose a different one if you find it confusing.

Another important aspect when it comes to determining RF redundancy is that  `eliminate_reachability_fences()` takes all users into account while `optimize_reachability_fences()` trusts only `ReachabilityFence`s.

> src/java.base/share/classes/java/lang/ref/Reference.java line 662:
> 
>> 660:      * @since 9
>> 661:      */
>> 662:     @IntrinsicCandidate
> 
> Sounds like we also want to restore `@DontInline` to cover the case when intrinsic is not available / disabled for some compiler. I vaguely remember some intrinsic handling code checks whether method is prohibited from inlining (maybe affects only global `-XX:-Inline`, not sure), so it might be as straightforward.

I'd like to use `-XX:DisableIntrinsic=_Reference_reachabilityFence` to switch to current behavior (no fence).
Also, `@DontInline` would require special handling in C1 to unconditionally inline it.

`@ForceInline` was there primarily to communicate the interaction with JVM. (Existing inlining heuristics should just unconditionally inline empty methods.) Once `@IntrinsicCandidate` is there, I don't see much value in any other annotations.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103490049
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103488472
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103480961
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103482636
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103418703
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103447864
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103457174
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2103467390