RFR: 8345067: C2: enable implicit null checks for ZGC reads [v2]

[Roberto Castañeda Lozano]

On Thu, 8 May 2025 09:51:44 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roberto Castañeda Lozano has updated the pull request incrementally with nine additional commits since the last revision:
>> 
>>  - Generalize tests by removing requires annotation and adding local applyIf rules
>>  - Assert that we do not move control nodes
>>  - Extend comment about hoisting DecodeN inputs
>>  - Apply Emanuels suggestions to ensure_node_is_at_block_or_above
>>  - Rename auxiliary functions
>>  - Rename auxiliary functions
>>  - Clarify scope of move_into
>>  - Extend comment about MachTemp nodes
>>  - Extract and reuse legitimize_address test
>
> src/hotspot/cpu/aarch64/gc/z/z_aarch64.ad line 130:
> 
>> 128:              Address::offset_ok_for_immed(ref_addr.offset(), exact_log2(size)),
>> 129:              "an instruction that can be used for implicit null checking should emit the candidate memory access first");
>> 130:       ref_addr = __ legitimize_address(ref_addr, size, rscratch2);
> 
> For context:
> 
>    132   /* Sometimes we get misaligned loads and stores, usually from Unsafe
>    133      accesses, and these can exceed the offset range. */
>    134   Address legitimize_address(const Address &a, int size, Register scratch) {                                                                                                                                         
>    135     if (a.getMode() == Address::base_plus_offset) {
>    136       if (! Address::offset_ok_for_immed(a.offset(), exact_log2(size))) {
>    137         block_comment("legitimize_address {");
>    138         lea(scratch, a);
>    139         block_comment("} legitimize_address");
>    140         return Address(scratch);
>    141       }
>    142     }
>    143     return a;
>    144   }
> 
> I wonder if it might be worth to create a `legitimize_address_requires_lea` that does the checks. Then you could refactor `legitimize_address` with it, and also use it here. Not sure if it is worth it, but it could ensure that the checks stay in sync. Up to you.

Thanks, done (commit 5c7da867).

> What about the `MachTemp`?

I did not include moving incoming MachTemp nodes so that I could reuse the function across `PhaseCFG::implicit_null_check` without risking behavioral changes. I extended the comment of `move_into` to clarify its scope (commit d6a749e4).

> Also: how specific to implicit null checks are your methods `move_into` and `maybe_hoist_into`? If they are not reusable elsewhere, it may be good to give them a more specific name.

I changed the name according to your suggestion below, except using "above" instead of "before" which I find more natural when referring to the dominator tree (commits dbe46110 and bcf08f90).

> src/hotspot/share/opto/lcm.cpp line 356:
> 
>> 354:       if (mach->in(j)->is_MachTemp()) {
>> 355:         assert(mach->in(j)->outcnt() == 1, "MachTemp nodes should not be shared");
>> 356:         // Ignore MachTemp inputs, they can be safely hoisted with the candidate.
> 
> Suggestion:
> 
>         // Ignore MachTemp inputs, they can be safely hoisted with the candidate.
>         // MachTemp have no inputs themselves and are only there to reserve a scratch
>         // register for the GC barrier of the memory operation.
> 
> That was what you told me in our offline meeting, I thought it was helpful context information.

Thanks, added a slightly generalized version (commit 446649a6).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087177975
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087179887
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087178324