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

[Vladimir Ivanov]

On Mon, 8 Dec 2025 09:45:43 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   IR test cases
>
> src/hotspot/share/opto/phaseX.cpp line 2051:
> 
>> 2049:     //   java -XX:VerifyIterativeGVN=1000 -Xcomp -XX:+StressReachabilityFences
>> 2050:     return false;
>> 2051:   }
> 
> Is this still true?

Good catch. I double-checked that IGVN verification doesn't fail anymore. Removed.

> src/hotspot/share/opto/reachability.cpp line 73:
> 
>> 71:  *
>> 72:  * After loop opts are over, it becomes possible to reliably enumerate all interfering safe points and
>> 73:  * to ensure that the referent is present in their oop maps.
> 
> For the sake of someone else who has to fix a bug here, or considers changing the design, can you please:
> - Give a more concrete example, e.g: load of a native memory address is hoisted, ... explain that this means we move it over the SafePoint in the backedge, and why this is problematic.
> - State your assumption / invariants that should hold after loop opts, that guarantee that it is safe to now attach to SP instead of RF. This one makes me a bit nervous, because it is another implicit assumption in C2, but I suppose we just have to live with that. But at least we can document it well ;)

Updated the comment.

> src/hotspot/share/opto/reachability.cpp line 166:
> 
>> 164:     lpt->_reachability_fences = new Node_List();
>> 165:   }
>> 166:   lpt->_reachability_fences->push(new_rf);
> 
> This code is duplicated elsewhere. Consider refactoring it with a `lpt->reachability_fences_push` method that automatically allocates the new `Node_List`.

Done.

> src/hotspot/share/opto/reachability.cpp line 216:
> 
>> 214:   // ResourceMark rm; // NB! not safe because insert_rf may trigger _idom reallocation
>> 215:   Unique_Node_List redundant_rfs;
>> 216:   GrowableArray<Pair<Node*,Node*>> worklist;
> 
> Tech debt alarm ;)
> 
> We should probably more `_idom` to a different arena then, right?

Agree. Will update it once #28581 lands in the repo.

> src/hotspot/share/opto/reachability.cpp line 473:
> 
>> 471:       if (extra_edge != nullptr) {
>> 472:         sfpt->add_req(extra_edge); // Add valid_length_test_input edge back
>> 473:       }
> 
> Could it be that you have two meanings for "extra edge" here? OR does the top comment:
> 
>> Turn extra safepoint edges into reachability fences
> 
> match with this?
> 
>> sfpt->add_req(extra_edge); // Add valid_length_test_input edge back
> 
> Again: mixing up these edges really feels like tech debt. We should fix that soon.

I expanded the comment to clarify what happens there. Hope it makes it clearer.

"extra edge" term is broader than "reachability edge" and ambiguity comes from the fact that non-debug edges are untyped, but there are multiple unrelated cases when edges are appended to safepoints.

> test/hotspot/jtreg/compiler/c2/TestReachabilityFenceFlags.java line 48:
> 
>> 46:  *                   -XX:+StressReachabilityFences -XX:+OptimizeReachabilityFences -XX:+PreserveReachabilityFencesOnConstants
>> 47:  *                     compiler.c2.TestReachabilityFenceFlags
>> 48:  */
> 
> Conside dropping `-Xcomp -XX:-TieredCompilation`, because we will run with that at some point in our CI anyway. Would give more options for different kinds of compilation, right?

It is intended as a smoke test for different RF-related control flags. It runs a very limited amount of testing (-Xcomp and C2-only with an empty main method) irrespective of execution mode.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2738887078
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2738887240
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2738887640
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2738887791
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2738888183
PR Review Comment: https://git.openjdk.org/jdk/pull/25315#discussion_r2738888293