RFR 8232992: Shenandoah: Implement self-fixing interpreter LRB

[Zhengyu Gu]

On 10/25/19 10:48 AM, Aleksey Shipilev wrote:
> On 10/25/19 4:29 PM, Zhengyu Gu wrote:
>> Please review this patch that implements self-fixing interpreter LRB.
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8232992
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8232992/webrev.00/
> 
> *) I believe we can drop "fixup" from ShenandoahRuntime::load_reference_barrier_fixup(_narrow), as
> there no not-fixup versions left.

Sure.

> 
> *) shenandoahBarrierSetAssembler_x86.cpp nit: space before parenthesis
> 
>   346   if(setup_addr_first) {

Fixed

> 
> *) shenandoahBarrierSetAssembler_x86.cpp: why pop(thread) is in the middle here?
> 
>   315   __ testb(gc_state, ShenandoahHeap::HAS_FORWARDED);
>   316 #ifndef _LP64
>   317   __ pop(thread);
>   318 #endif
>   319   __ jccb(Assembler::zero, done);
> 

I was worried about having to track 'thread', so just pop it after use 
and forget about it.

But, yes, there is nothing to worry about, reverted.

> *) shenandoahBarrierSetAssembler_x86.cpp: seems to me it is cleaner to initialize the boolean
> variable first, and then use it. Also, suggestion for name: "need_addr_setup".
> 
>      // Use rsi for src address
>      const Register src_addr = rsi;
>      bool need_addr_setup = (src_addr != dst);
> 
>      if (need_addr_setup) {
>        ...
>      } else {
>        ...
>      }
> 
>      __ call(RuntimeAddress(CAST_FROM_FN_PTR(...);
> 
>      if (need_addr_setup) {
>        ...
> 
Fixed

> *) shenandoahBarrierSetAssembler_x86.cpp, shenandoahBarrierSetAssembler_aarch64.cpp: since this code
> now uses rscratch1, it has to assert that registers do not clash. For example with:
> 
>     assert_different_registers(dst, rscratch1, rscratch2);

We only need to use rscratch1 when dst == r1, and there is possibility 
that dst comes in in rscratch1 (see SBSA::load_at() method), I think 
current assertion (dst != rscratch2) is sufficient.

However, we do need to ensure scratch registers are not used by 
load_addr, so added:

   assert_different_registers(load_addr.base(), load_addr.index(), 
rscratch1);
   assert_different_registers(load_addr.base(), load_addr.index(), 
rscratch2);


Updated: http://cr.openjdk.java.net/~zgu/JDK-8232992/webrev.01/

Reran hotspot_gc_shenandoah tests (fastdebug and release)
   x86_64 and x86_32 on Linux
   aarch64 on Linux

Thanks,

-Zhengyu

> 
> *) shenandoahBarrierSetC2.cpp: this change looks like a bug fix for matching _narrow. Please RFR it
> separately, it should go in sooner.
>