RFR: 8334060: Implementation of Late Barrier Expansion for G1 [v13]

[Martin Doerr]

On Thu, 5 Sep 2024 10:07:14 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/gc/g1/g1_aarch64.ad line 646:
>> 
>>> 644: instruct g1LoadPVolatile(iRegPNoSp dst, indirect mem, iRegPNoSp tmp1, iRegPNoSp tmp2, rFlagsReg cr)
>>> 645: %{
>>> 646:   predicate(UseG1GC && needs_acquiring_load(n) && n->as_Load()->barrier_data() != 0);
>> 
>> Remark: This node should never match because the `referent` is never volatile (same for `g1LoadNVolatile`): https://github.com/openjdk/jdk/blob/7a418fc07464fe359a0b45b6d797c65c573770cb/src/java.base/share/classes/java/lang/ref/Reference.java#L157
>> Hence, `needs_acquiring_load(n)` and `n->as_Load()->barrier_data() != 0` are never true at the same time.
>> Not sure if this should somehow be reflected in the code. I've inserted `ShouldNotReachHere` on PPC64.
>
> Good catch, thanks! I simply removed the `g1LoadXVolatile` patterns and added a comment explaining why they are not needed (commit 9821e795). The matcher should already fail if we ever end up with an erroneous `LoadX` node `n` for which `UseG1GC && needs_acquiring_load(n) && n->as_Load()->barrier_data() != 0` holds.

Correct. Only the error message may be not so nice ("bad AD file").
PPC64 still has `g1LoadP_acq` and `g1LoadN_acq` which could also be replaced by a comment. But it's not important.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1745230285