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 shenandoah-dev
mailing list