[aarch64-port-dev ] openjdk atomics

[Edward Nevill]

On Fri, 2014-02-14 at 17:08 +0000, Andrew Haley wrote:
> On 02/14/2014 03:55 PM, Andrew Haley wrote:
> Here it is; it was a silly thinko.
> 
> Andrew.

Hi Andrew,

This looks fine now, there are a few points below, but they are only minor optimisation points.

Regards,
Ed.


>      Label retry_load, done;
> +    __ membar(__ AnyAny);
>      __ bind(retry_load);
> -    __ ldaxr(rscratch1, addr_reg);
> +    __ ldxr(rscratch1, addr_reg);
>      __ cmp(rscratch1, old_reg);
>      __ br(Assembler::NE, done);
>      // if we store+flush with no intervening write tmp will be zero
> -    __ stlxr(rscratch1, new_reg, addr_reg);
> +    __ stxr(rscratch1, new_reg, addr_reg);
>      __ cmpw(rscratch1, zr);       // cannot use cbzw as must set flag
>      __ br(Assembler::EQ, done);
>      // retry so we only ever return after a load fails to compare
>      // ensures we don't return a stale value after a failed write.
>      __ b(retry_load);
>      __ bind(done);

Despite the comment saying "cannot use cbzw" here I believe we can use cbzw. We know condition EQ is already true from the 'BNE done' previously. Therefore the last three instructions can be replaced with

 __ cbnzw(rscratch1, retry_load);
 // Fall thru with condition EQ set
 __ bind(done);

> atch1, new_reg, addr_reg);
> +    __ stxrw(rscratch1, new_reg, addr_reg);
>      __ cmpw(rscratch1, zr);       // cannot use cbzw as must set flag
>      __ br(Assembler::EQ, done);
>      // retry so we only ever return after a load fails to compare
>      // ensures we don't return a stale value after a failed write.
>      __ b(retry_load);
>      __ bind(done);

Ditto for this one.

> diff -r a69fcb4e97f6 -r 9358a801c67a src/cpu/aarch64/vm/c1_LIRAssembler_aarch64.cpp
> --- a/src/cpu/aarch64/vm/c1_LIRAssembler_aarch64.cpp	Tue Feb 11 15:36:54 2014 +0000
> +++ b/src/cpu/aarch64/vm/c1_LIRAssembler_aarch64.cpp	Fri Feb 14 16:25:23 2014 +0000
> @@ -1580,33 +1580,37 @@
>    // flush and load exclusive from the memory location
>    // and fail if it is not what we expect
>    __ bind(retry_load);
> -  __ ldaxrw(rscratch1, addr);
> +  __ membar(__ AnyAny);

The membar can be placed outside the loop. IE

 __ membar(__ AnyAny);
 __ bind(retry_load);

This is what is done in other cases.


>  void LIR_Assembler::casl(Register addr, Register newval, Register cmpval) {
>    Label retry_load, nope;
>    // flush and load exclusive from the memory location
>    // and fail if it is not what we expect
> +  __ membar(__ AnyAny);
>    __ bind(retry_load);

EG. Here


I have no other comments. It is fine to push either as is, or with the few minor changes above.

Regards,
Ed.