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

[Hamlin Li]

On Mon, 30 Sep 2024 05:02:12 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> This changeset implements JEP 475 (Late Barrier Expansion for G1), including support for the x64 and aarch64 platforms. See the [JEP description](https://openjdk.org/jeps/475) for further detail.
>> 
>> We aim to integrate this work in JDK 24. The purpose of this pull request is double-fold:
>> 
>> - to allow maintainers of the arm (32-bit), ppc, riscv, s390, and x86 (32-bit) ports to contribute a port of these platforms in time for JDK 24; and
>> - to allow reviewers to review the platform-independent, x64 and aarch64, and test changes in parallel with the porting work.
>> 
>> ## Summary of the Changes
>> 
>> ### Platform-Independent Changes (`src/hotspot/share`)
>> 
>> These consist mainly of:
>> 
>> - a complete rewrite of `G1BarrierSetC2`, to instruct C2 to expand G1 barriers late instead of early;
>> - a few minor changes to C2 itself, to support removal of redundant decompression operations and to address an OopMap construction issue triggered by this JEP's increased usage of ADL `TEMP` operands; and
>> - temporary support for porting the JEP to the remaining platforms.
>> 
>> The temporary support code (guarded by the pre-processor flag `G1_LATE_BARRIER_MIGRATION_SUPPORT`) will **not** be part of the final pull request, and hence does not need to be reviewed.
>> 
>> ### Platform-Dependent Changes (`src/hotspot/cpu`)
>> 
>> These include changes to the ADL instruction definitions and the `G1BarrierSetAssembler` class of the x64 and aarch64 platforms.
>> 
>> #### ADL Changes
>> 
>> The changeset uses ADL predicates to force C2 to implement memory accesses tagged with barrier information using G1-specific, barrier-aware instruction versions (e.g. `g1StoreP` instead of the GC-agnostic `storeP`). These new instruction versions generate machine code accordingly to the corresponding tagged barrier information, relying on the G1 barrier implementations provided by the `G1BarrierSetAssembler` class. In the aarch64 platform, the bulk of the ADL code is generated from a higher-level version using m4, to reduce redundancy.
>> 
>> #### `G1BarrierSetAssembler` Changes
>> 
>> Both platforms basically reuse the barrier implementation for the bytecode interpreter, with the different barrier tests and operations refactored into dedicated functions. Besides this, `G1BarrierSetAssembler` is extended with assembly-stub routines that implement the out-of-line, slow path of the barriers. These routines include calls from the barrier into the JVM, which require support for saving and restoring live ...
>
> Roberto Castañeda Lozano has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 53 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'feilongjiang/JEP-475-RISC-V' into JDK-8334060-g1-late-barrier-expansion
>  - riscv port refactor
>  - Remove temporary support code
>  - Merge jdk-24+17
>  - Relax 'must match' assertion in ppc's g1StoreN after limiting pinning bypass optimization
>  - Remove unnecessary reg-to-reg moves in aarch64's g1CompareAndX instructions
>  - Reintroduce matcher assert and limit pinning bypass optimization to non-shared EncodeP nodes
>  - Merge jdk-24+16
>  - Ensure that detected encode-and-store patterns are matched
>  - Merge remote-tracking branch 'snazarkin/arm32-JDK-8334060-g1-late-barrier-expansion' into JDK-8334060-g1-late-barrier-expansion
>  - ... and 43 more: https://git.openjdk.org/jdk/compare/9566d51f...14483b83

Hi, have some comments on riscv part code.
I'm not sure if the same comments also apply to other code, please have a look if necessary.

src/hotspot/cpu/riscv/gc/g1/g1_riscv.ad line 55:

> 53:   }
> 54:   for (RegSetIterator<Register> reg = no_preserve.begin(); *reg != noreg; ++reg) {
> 55:     stub->dont_preserve(*reg);

Could `no_preserve` and `preserve` overlap?
If false, then seems it's not necessary to do `dont_preserve` for `no_preserve`
If true, seems it's not safe to `dont_preserve` these regs? I'm not sure.

src/hotspot/cpu/riscv/gc/g1/g1_riscv.ad line 169:

> 167:   predicate(UseG1GC && n->as_LoadStore()->barrier_data() != 0);
> 168:   match(Set res (CompareAndExchangeP mem (Binary oldval newval)));
> 169:   effect(TEMP res, TEMP tmp1, TEMP tmp2);

should `res` be `TEMP_DEF`?

src/hotspot/cpu/riscv/gc/g1/g1_riscv.ad line 201:

> 199:   predicate(UseG1GC && needs_acquiring_load_reserved(n) && n->as_LoadStore()->barrier_data() != 0);
> 200:   match(Set res (CompareAndExchangeP mem (Binary oldval newval)));
> 201:   effect(TEMP res, TEMP tmp1, TEMP tmp2);

should `res` be `TEMP_DEF`?

src/hotspot/cpu/riscv/gc/g1/g1_riscv.ad line 233:

> 231:   predicate(UseG1GC && n->as_LoadStore()->barrier_data() != 0);
> 232:   match(Set res (CompareAndExchangeN mem (Binary oldval newval)));
> 233:   effect(TEMP res, TEMP tmp1, TEMP tmp2, TEMP tmp3);

should `res` be `TEMP_DEF`?

src/hotspot/cpu/riscv/gc/g1/g1_riscv.ad line 263:

> 261:   predicate(UseG1GC && needs_acquiring_load_reserved(n) && n->as_LoadStore()->barrier_data() != 0);
> 262:   match(Set res (CompareAndExchangeN mem (Binary oldval newval)));
> 263:   effect(TEMP res, TEMP tmp1, TEMP tmp2, TEMP tmp3);

should `res` be `TEMP_DEF`?

And same comment for following instructs?

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

PR Review: https://git.openjdk.org/jdk/pull/19746#pullrequestreview-2342455263
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1784240549
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1784209154
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1784210589
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1784211728
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1784212185