[aarch64-port-dev ] RFR(XS): 8211320: Aarch64: unsafe.compareAndSetByte() and unsafe.compareAndSetShort() c2 intrinsics broken with negative expected value

[Andrew Dinn]

On 01/10/18 12:03, Andrew Haley wrote:
> On 10/01/2018 11:50 AM, Andrew Dinn wrote:
>> On 01/10/18 11:43, Andrew Haley wrote:
>> You seem to have answered that question (correctly?) in the negative :-)
> 
> IMO it's more complicated than that.
> 
>> If you moved the sign extend into the cmpxchg method the C2 code would
>> still have to copy the input to a temporary register and pass the temp
>> into cmpxchg. Since this is the only place where a cmpxchgb actually
>> occurs that doesn't seem like a better solution.
> 
> The problem is that cmpxchg() is a trap for the unwary.
> 
> The alternative would be to have cmpxchg itself do the sign extension
> on the output. I guess it all depends on what the LSE instructions do,
> given that we don't want to have different results on a machine that
> supports LSE.
Just to be precise here I should, of course, have said 'if you moved the
/unsigned/ extend into the cmpxchg method ...'

I take your point that this is a trap because as currently coded cmpxchg
will load an unsigned byte or half-word and perform a 32-bit compare
against the value held in register oldval. Now, that's only a trap
because byte and half-word values are conventionally passed around
internally in registers as /signed/ 32-bit values. Normally, loads and
stores enforce the necessary unsigned to signed conversion. So, in this
one case, where the load does not (cannot) do sign extension, in order
to get the correct comparison the caller or callee must massage the
register value with an unsigned extend before calling cmpxchg. So, what
I had assumed you were suggesting was that the callee, cmpxchg, should
perform the unsigned extension on the input value.

However, it appears that unsigned extension, whether by caller or
callee, is not the right fix because it does not deal with a related
problem. In the current usage result is passed as noreg. However, if
result were supplied as a writeable register, to be assigned the loaded
byte or short value, then that value would be handed out /unsigned/,
breaking the convention that loaded byte or short values are installed
into a register sign-extended. So, the correct fix is to sign-extend
result after the load if needed i.e.

    . . .
    bind(retry_load);
    load_exclusive(result, addr, size, acquire);
    if (size == byte) {
      sxtbw(result, result);
    } else if (size == byte) {
      sxthw(result, result);
    }
    if (size == xword)
      cmp(result, expected);
    else {
      cmpw(result, expected);
    br(Assembler::NE, done);
    . . .

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