RFR: 8217874: Shenandoah: AArch64: Clobbered register in ShenandoahBarrierSetAssembler::cmpxchg_oop()

[Andrew Dinn]

On 11/02/2019 13:16, Roman Kennke wrote:
>>>> In AArch64, when called from C2, in
>>>> ShenandoahBarrierSetAssembler::cmpxchg_oop() the result register may
>>>> overlap with other input argument registers and thus fail the leading
>>>> assert, and lead to clobbered registers. In the body of the code block,
>>>> a temporary register should be used instead, and result should only get
>>>> filled in at the end.
>>
>> That looks convincing except that the original code avoided using
>> rscratch1. Was there a reason for that?
>>
>> It may be that way because the original cmpxchg code that this is
>> derived from allowed that callers might be holding something in
>> rscratch1 across the cmpxchg call. Will that ever happen for your usage
>> in the barrier set?
> 
> I believe this is before we used rscratch1 for holding the result in
> case of not generating for compare-and-exchange, but this changed with:
> http://hg.openjdk.java.net/jdk/jdk/rev/74a5ef4c81cc

Ok, so using rscratch1 looks fine then.

>> If you are sure that this won't happen and that all the gubbins you call
>> from the barrier cmpxchg_oop implementation doesn't use rscratch1 then I
>> think this is ok.
> 
> Yeah I'm pretty sure.
> 
>> Of course, if the result/input overlap is only ever happening in calls
>> from C2 then you could fix this more simply in the C2 rules. Allocate a
>> temp register in each rule that encodes a call to cmpxchg_oop, pass that
>> temp register in as the 'res' argument and then plant a move to the
>> output register after the call to cmpxchg_oop.
> 
> I thought about that too. But then, why allocate an additional register,
> when we can have one for free?
Yes, using rscratch1 is better because it avoids an extra register
allocate. So, your patch is reviewed.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander