RFR: 8329597: C2: Intrinsify Reference.clear [v8]

[Aleksey Shipilev]

On Mon, 7 Oct 2024 08:15:21 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   More precise bit-unmasks
>
> src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 342:
> 
>> 340:   }
>> 341:   if ((on_weak || on_phantom) && no_keepalive) {
>> 342:     // Be extra paranoid around this path. Only accept null stores,
> 
> I think there might be some orthogonal stuff that is unnecessarily mixed up here. When no_keepalive is manually specified, then we shouldn't do the pre-write barrier, regardless of reference strength. Similarly, when the new value is null, we don't need to perform the post write barrier, regardless of reference strength. Roberto added some code in refine_barrier_by_new_val_type that already *should* take care of the latter part. It allows types to flow around a bit, and then checks if the type of the new value is provably null, and then removes the post write barrier. The existing logic for that should be strictly more powerful than the new check you added, I think.
> 
> Based on the above explanation, I think I'm proposing this block is replaced with this simpler condition:
> 
> if (no_keepalive) {
>   access.set_barrier_data(access.barrier_data() & ~G1C2BarrierPre);
> }

Right. We also do not need this complexity in Shenandoah barriers. This check was dragged here from the load barriers that _want_ to check if we are reading the `Reference.referent` and feed it to SATB _unless_ there is a no-keep-alive. For store barriers it is unnecessary, and we can just do keep-alive checks straight up. Should be done in new commit, testing now.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20139#discussion_r1793115683