RFR: JDK-8221766: Load-reference barriers for Shenandoah

Roman Kennke rkennke at redhat.com
Thu Apr 4 14:56:59 UTC 2019


>> Additional testing on aarch64 showed two problems there:
>> - we must not use rscratch1 in the LRB asm code, there is at least one
>> place where the LRB is called with dst==rscratch1.
> 
> And it might be called with dst==rscratch2, right? You have to code
> defensively against that. Or at least assert it.

Yes, we do have an assert there:

   assert(dst != rscratch2, "need rscratch2");

>> - we need enter()/leave() around the barrier because this is now called
>> in some places that don't have a proper frame, yet.
>>
>> The fix for the rscratch1 problem also simplifies and streamlines the
>> LRB to be shaped like the C2 generated LRB. It checks for HAS_FORWARDED
>> gc-state, and then dives into the stub, which 1. checks obj in cset, 2.
>> resolves the fwd ptr, 3. compares obj == fwd, and only then dives into
>> slow-path.
> 
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8221766
>>> Webrev:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8221766/webrev.00/
>>>
>>> Can I please get reviews for this change?
> 
> One thing:
> 
>     Register obj = r0;
> +  __ mov(rscratch2, r0);
> +  resolve_forward_pointer_not_null(cgen->assembler(), r0);
> +  __ cmp(rscratch2, r0);
> +  __ br(Assembler::NE, done);
> 
> The macro assembler convention is that rscratchX is never preserved
> across a macro, even if you wrote that macro yourself. I've been
> trying to clean up these as I go, because this problem has bitten us
> repeatedly. I guess you know that whatever is called by
> resolve_forward_pointer_not_null(cgen->assembler(), r0) is okay...

Yeah... In this case, resolve_forward_pointer_not_null() is really just 
a single instruction (the load of the fwd ptr into the same register) 
and I know it is not trashing anything else. I could expand that load 
right there to make it more obvious, however this might become more 
complex soon when we eliminate the fwd ptr. But even then, I will make 
sure that it will still be a single-register-macro.

> Also, if you're going to say
> 
>     Register obj = r0;
> 
> please either use "obj" everywhere or "r0", not a mixture.
> I'd just get rid of the Register alias.
> 

That sounds reasonable.

Incremental:
http://cr.openjdk.java.net/~rkennke/JDK-8221766/webrev.03.diff/
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8221766/webrev.03/

Ok to push it like this?

Roman




More information about the hotspot-gc-dev mailing list