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