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

[Roberto Castañeda Lozano]

On Thu, 8 May 2025 10:48:26 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/share/opto/lcm.cpp line 79:
> 
>> 77: }
>> 78: 
>> 79: void PhaseCFG::move_into(Node* n, Block* b) {
> 
> Suggestion:
> 
> void PhaseCFG::move_node_and_its_projections_to_block(Node* n, Block* b) {

Done.

> Do I understand this right: You are looking at some input `n` here, and want to make sure that it is located at `b` or before?

Right.

> I did not understand what this meant: `sanity check: temp node placement`... Ah, I suppose we are assuming that `n` is a `MachTemp`, and this would have to be placed in a block dominated by b? But could `n` not also be a `load Base`? Could that be a `MachProj`? Just a little confused here. Maybe moving the `b->dominates(current)` assert down helps give good context? But in a sense, it is also a precondition, we can only move `n` up to `b` if `b` dominates `n`...
> 
> Do you have a better idea?

Right, the comment comes from the original context from which the code is moved, and I guess it should be generalized to make more sense in its new context. I went with your suggestion (commit 793bbe7f). The intention of `ensure_node_is_at_block_or_above` becomes hopefully clear by looking at its callees.

> 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).

> src/hotspot/share/opto/lcm.cpp line 437:
> 
>> 435:     if (n == nullptr || !n->is_MachTemp()) {
>> 436:       continue;
>> 437:     }
> 
> Do you want to check that all other nodes already dominate `block`?

This is guaranteed by the input domination test in https://github.com/openjdk/jdk/pull/25066/files#diff-6343a8024ec7abfc1bd5e377ba254ed868d97a99258b5af0aab12ecf8f961503R345-R369, so it feels a bit redundant. Let me know if you still think it would be useful to add the assertion.

> src/hotspot/share/opto/lcm.cpp line 439:
> 
>> 437:     }
>> 438:     maybe_hoist_into(n, block);
>> 439:   }
> 
> It seems to me this is definitely new code, ensuring that we move the `MachTemp`. We did not do that before, at least not here. Correct?

That's right.

> src/hotspot/share/opto/lcm.cpp line 441:
> 
>> 439:       map_node_to_block(n, block);
>> 440:     }
>> 441:   }
> 
> This now happens in `move_into`, right?

Yes.

> src/hotspot/share/opto/machnode.hpp line 391:
> 
>> 389: 
>> 390:   // Whether this node is expanded during code emission into a sequence of
>> 391:   // instructions and the first instruction can perform an implicit null check.
> 
> You may want to put a warning / reasoning here, in case there are multiple loads.
> You explained to me offline that a `zLoadP` may have a load at the beginning, but then need to load again if the GC moved the object. I suppose if it was moved, then it cannot be null, and so that should be safe... maybe that is a sufficient argument, what do you think?

In light of our discussion above I am not sure this warning is needed, the key invariant IMO is that the very first instruction emitted should be able to implement the implicit null check.

> test/hotspot/jtreg/compiler/gcbarriers/TestImplicitNullChecks.java line 51:
> 
>> 49:  * @requires vm.gc.Z
>> 50:  * @run driver compiler.gcbarriers.TestImplicitNullChecks Z
>> 51:  */
> 
> Do you think there would be any value in having a run without requirements? Just for general result verification... i.e. that we get the correct NullPointerException.
> Of course, you would have to probably add `applyIf` to the `@IR` rules.

Sure, done (commit 6353f42b).

> test/hotspot/jtreg/compiler/gcbarriers/TestImplicitNullChecks.java line 140:
> 
>> 138:     // G1 and ZGC stores cannot be currently used to implement implicit null
>> 139:     // checks, because they expand into multiple memory access instructions that
>> 140:     // are not necessarily located at the initial instruction start address.
> 
> Very random idea, no idea if it is any good:
> Why not do the implicit null-check with a fake Load?
> No idea on the implications here. I suppose it would be extra code, but at least not branching code?

Thanks, but given that doing something theoretically more efficient (addressing the limitation and using the stores for implicit null checking as described in https://github.com/openjdk/jdk/pull/25066#issuecomment-2872870543) did not show any benefit in practice I would not expect any benefit either from implementing the null checks with a synthetic load.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087184637
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087184187
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087195811
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087187130
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087191174
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087187408
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087187699
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087188494
PR Review Comment: https://git.openjdk.org/jdk/pull/25066#discussion_r2087190800