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

[Roberto Castañeda Lozano]

On Tue, 3 Sep 2024 12:04:09 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - Increase test coverage of new-object stores with different type information
>>  - Refactor the two post-barrier removal cases into a single expression
>>  - Remove unnecessary early null-based post-barrier elision
>>  - Make store capturability test G1-specific and more precise
>
> 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.

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

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