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

[Martin Doerr]

On Tue, 13 Aug 2024 14:21:01 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp line 203:
>> 
>>> 201:   // Do we need to load the previous value?
>>> 202:   if (obj != noreg) {
>>> 203:     __ load_heap_oop(pre_val, Address(obj, 0), noreg, noreg, AS_RAW);
>> 
>> How do we handle implicit null checks for which `obj` is null? Note that we may expect the store instruction to trigger SIGSEGV. Does it work correctly if we trigger the SIGSEGV, here in the pre barrier?
>
> Good question! I have checked (no pun intended) and it turns out C2 never uses stores (or other memory access operations) with late-expanded barriers to perform implicit null checks. This is accidental, due to the fact that all these memory operations use MachTemp nodes in the C2 code generation stage (to reserve registers for their ADL TEMP operands). C2's implicit null check analysis requires that all inputs of a candidate memory operation dominate the null check [1], which fails if the operation uses MachTemp nodes, since these are always placed in the same basic block [2].
> 
> Note that this optimization triggers very rarely, if at all, for memory operations in the current early barrier expansion model, since the additional control flow of the barrier code obfuscates the analysis. For late barrier expansion, the analysis could be easily extended to recognize and hoist MachTemp nodes together with their user memory operation that is a candidate to implement the implicit null check [3], but that would require extending the barrier assembly emission step to populate the implicit null exception table correctly. Since this seems non-trivial, and would also affect other garbage collectors (ZGC), I suggest to simply assert for now that we do not generate implicit null checks for memory operations with barrier data (as in [4]), and leave full support for implicit null checks for these G1 and ZGC operations to a future RFE. What do you think?
> 
> [1] https://github.com/robcasloz/jdk/blob/d21104ca8ff1eef88a9d87fb78dda3009414b5b8/src/hotspot/share/opto/lcm.cpp#L310-L328
> [2] https://github.com/robcasloz/jdk/blob/d21104ca8ff1eef88a9d87fb78dda3009414b5b8/src/hotspot/share/opto/gcm.cpp#L1397-L1404
> [3] https://github.com/robcasloz/jdk/commit/e0ab1e418b81c0acddff2190ca57b3335b5214ba
> [4] https://github.com/robcasloz/jdk/commit/e0ab1e418b81c0acddff2190ca57b3335b5214ba#diff-554fddca91406a67dc0f8faee12dc30c709181685a0add7f4ba9ae5ace68f192R2031-R2032

Thanks for figuring it out! Makes sense.

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

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