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

[Vladimir Kozlov]

On Wed, 11 Sep 2024 08:30:02 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 incrementally with one additional commit since the last revision:
> 
>   Fix a few style issues

src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 241:

> 239:   assert(newval_bottom->isa_ptr() || newval_bottom->isa_narrowoop(), "newval should be an OOP");
> 240:   TypePtr::PTR newval_type = newval_bottom->make_ptr()->ptr();
> 241:   uint8_t barrier_data = store->barrier_data();

Should you check barrier data for 0?
`is_ptr()` has wide set of types. It includes TypeRawPtr, TypeKlassPtr and TypeMetadataPtr. Where you filtering them?

src/hotspot/share/gc/g1/g1BarrierSet.cpp line 65:

> 63: #else
> 64:                       make_barrier_set_c2<G1BarrierSetC2>(),
> 65: #endif

I assume it is temporary until all ports a ready (except 32-bit x86 may be). Right?

src/hotspot/share/opto/matcher.cpp line 1821:

> 1819:   if( rule >= _END_INST_CHAIN_RULE || rule < _BEGIN_INST_CHAIN_RULE ) {
> 1820:     assert(C->node_arena()->contains(s->_leaf) || !has_new_node(s->_leaf),
> 1821:            "duplicating node that's already been matched");

Why it was removed?

src/hotspot/share/opto/matcher.cpp line 2845:

> 2843:     n->Opcode() == Op_StoreN &&
> 2844:     m->is_EncodeP();
> 2845: }

Add comment that `m` is input of `n`. I thought about adding assert too but I will leave it to you.

src/hotspot/share/opto/output.cpp line 2026:

> 2024:     if (n->is_MachNullCheck()) {
> 2025:       assert(n->in(1)->as_Mach()->barrier_data() == 0,
> 2026:              "Implicit null checks on memory accesses with barriers are not yet supported");

I don't see here changes in `lcm.cpp` which would prevent it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1759604325
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1759604944
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1759593453
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1759593131
PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1759605704