RFR: 8255762: Shenandoah: Consolidate/streamline interpreter LRBs

[Aleksey Shipilev]

On Mon, 2 Nov 2020 15:55:40 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

> We currently have two LRB implementations in interpreter: one normal and one for native/weak LRB. We should consolidate them into one.
> 
> The main issue here is that the two implementations follow different approaches:
> - the normal LRB calls through the shenandoah_lrb stub which does additional null- and cset-checking
> - the weak LRB calls to runtime directly and must not do cset-checking
> 
> The reason for calling through the stub is that it gives more freedom to allocate two registers that are required for the cset check. However, we can invert the cset addressing like we did in JDK-8245465 and save a register. We can also eliminate the null-check and let the cset-check subsume it (like we do everywhere else). Allocating a single register for the cset-check is easy, and we can do so in-line without the extra jump through the stub. The runtime call through the stub has also been very costly: it dumps 2KB of register data on the stack at each call, that is very excessive. save_xmm_registers() should be more than enough (in-fact, I am almost certain that this is excessive too, and we should only need to save/restore xmm0 - but not in this patch). Not needing to generate the call-stub is also helpful for backportability, because in jdk8-shenandoah we cannot do that.
> 
> Testing: hotspot_gc_shenandoah (x86_64, x86_32)

This looks nice. Please run `hotspot_gc_shenandoah` on `aarch64`?

src/hotspot/cpu/aarch64/gc/shenandoah/shenandoahBarrierSetAssembler_aarch64.cpp line 239:

> 237: 
> 238:   // Check for heap stability
> 239:   __ tbz(rscratch2, ShenandoahHeap::HAS_FORWARDED_BITPOS, not_fwded);

`not_fwded` reads as if the object is not forwarded. May it should be `heap_stable`?

src/hotspot/cpu/x86/gc/shenandoah/shenandoahBarrierSetAssembler_x86.cpp line 307:

> 305:     assert(tmp1 != dst, "");
> 306:     assert(tmp1 != src.base(), "");
> 307:     assert(tmp1 != src.index(), "");

Is this just `assert_different_registers(tmp1, dst, src.base(), src.index())`, or am I missing something?

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

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1010