[aarch64-port-dev ] RFR 8233401: Shenandoah: Refactor/cleanup Shenandoah load barrier code

[Andrew Haley]

On 11/2/19 3:07 PM, Zhengyu Gu wrote:
> Please review this refactor of Shenandoah load barrier. The goal is to 
> make the barrier structurally similar cross interpreter, C1 and C2, 
> improve readability and maintainability.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8233401
> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8233401/webrev.00/index.html
> 
> Test:
>    hotspot_gc_shenandoah (fastdebug and release)
>    x86_64 and x86_32 on Linux
>    AArch64 on Linux

Thanks, this is an improvement. However, it's still weird.

//
// Arguments:
//
// Inputs:
//   src:        oop location to load from, might be clobbered
//   tmp1:       unused
//   tmp_thread: unused
//
// Output:
//   dst:        oop loaded from src location
//
// Kill:
//   rscratch1 (scratch reg)
//
// Alias:
//   dst: rscratch1 (might use rscratch1 as temporary output register to avoid clobbering src)
//
void ShenandoahBarrierSetAssembler::load_at(MacroAssembler* masm, DecoratorSet decorators, BasicType type,
                                            Register dst, Address src, Register tmp1, Register tmp_thread) {

tmp1 and tmp_thread are unused? It'd be a good idea, then, to say if they are
safe to use or not. Or maybe even better do this if you want to keep the same
arg list:

void ShenandoahBarrierSetAssembler::load_at(MacroAssembler* masm, DecoratorSet decorators, BasicType type,
                                            Register dst, Address src, Register, Register) {

I guess it really isn't safe to use "tmp1" as a tmp, regardless of its name.

If so, better pass it as noreg/

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671