RFR: Backport SFX-related changes to sh/jdk8

Roman Kennke rkennke at redhat.com
Mon May 25 13:34:53 UTC 2020



> > http://cr.openjdk.java.net/~rkennke/sfx-jdk8/webrev.03/
> Cannot shake the feeling this is too risky for July release.
> 

Yeah, I've been thinking the same. Should I hold it back?

> *) We cannot pick up changesets like that:
>   8202976: Add C1 lea patching support for x86
> 
> ...because it messes up tracking. It should be something like:
>   Cherry-pick 8202976: Add C1 lea patching support for x86
> 

Right, fixed.

> *) src/cpu/aarch64/vm/c1_LIRAssembler_aarch64.cpp: the new block
> should be guarded by UseShenandoahGC?
> 

Yes, fixed too.

> *) src/cpu/x86/vm/c1_LIRAssembler_x86.cpp: I thought we were
> discussing how to isolate this with
> UseShenandoahGC? It is odd to change upstream LIR_Assembler::leal.
> 

Uff, how did that get lost? Anyhow, fixed.

> *) src/cpu/aarch64/vm/macroAssembler_aarch64.cpp
> 
> It looks to me Shenandoah interception is cleaner with early return
> like this:
> 
>  void MacroAssembler::load_heap_oop(Register dst, Address src)
>  {
>  #if INCLUDE_ALL_GCS
>    if (UseShenandoahGC) {
>      ShenandoahBarrierSetAssembler::bsasm()->load_heap_oop(this, dst,
> src);
>      return;
>    }
>  #endif
> 
>    if (UseCompressedOops) {
>      ldrw(dst, src);
>      decode_heap_oop(dst);
>    } else {
>      ldr(dst, src);
>    }
>  }
> 

Right, fixed this too.

> *) src/cpu/x86/vm/macroAssembler_x86.cpp: same as above. Should just
> use early return.
> 

Yup.

> *) src/cpu/x86/vm/shenandoahBarrierSetAssembler_x86.cpp: bad indent
> at L437, L468.
> 
>  430 static void load_at(MacroAssembler* masm, Register dst, Address
> src) {
>  431 #ifdef _LP64
>  432   if (UseCompressedOops) {
>  433     __ movl(dst, src);
>  434     __ decode_heap_oop(dst);
>  435   } else
>  436 #endif
>  437     __ movptr(dst, src);
>  438 }
> 
>  461 static void load_at_not_null(MacroAssembler* masm, Register dst,
> Address src) {
>  462 #ifdef _LP64
>  463   if (UseCompressedOops) {
>  464     __ movl(dst, src);
>  465     __ decode_heap_oop_not_null(dst);
>  466   } else
>  467 #endif
>  468     __ movptr(dst, src);
>  469 }
> 

This is pre-existing and not ours.

> *) src/share/vm/c1/c1_LIRGenerator.cpp, I do not understand this
> change:
> 
> -    if (is_volatile && !needs_patching) {
> -      volatile_field_load(address, tmp, info);
> +    if (is_volatile) {
> +      volatile_field_load(addr->as_address_ptr(), tmp, info);
>      } else {
> -      LIR_PatchCode patch_code = needs_patching ? lir_patch_normal :
> lir_patch_none;
> -      __ load(address, tmp, info, patch_code);
> +      __ load(addr->as_address_ptr(), tmp, info);
>      }
>      if (is_volatile && os::is_MP()) {
>        __ membar_acquire();
> 
> Does this mean we always call load(..., lir_patch_none), even when
> need_patching is true?
> 

Yes, that is right. The reason is that the address is lea-ed in the
LRB, and patching happens there. The subsequent load uses the pre-
computed and patched address. This mirrors how it's done in 11+, there
we are dropping the C1_NEEDS_PATCHING decorator after resolving the
address into a register w/ patch.

> *)
> src/share/vm/gc_implementation/shenandoah/shenandoahBarrierSetC1.cpp,
> this is odd:
> 
>  108 #ifdef AARCH64
>  109       // AArch64 expects double-size register.
>  110       obj_reg = gen->new_pointer_register();
>  111 #else
>  112       // x86 expects single-size register.
>  113       obj_reg = gen->new_register(T_OBJECT);
>  114 #endif
> 
> I thought we eliminated it with:
>   https://hg.openjdk.java.net/jdk/jdk/rev/732
> 
> 

I appended:

JDK-8238851: Shenandoah: C1: Resolve into registers of correct type

to my list of changes.

http://cr.openjdk.java.net/~rkennke/sfx-jdk8/webrev.04/

Re-run hotspot_gc_shenandoah on x86_64, all good.

Roman



More information about the shenandoah-dev mailing list