[aarch64-port-dev ] RFR: aarch64: minor improvements of atomic operations

[Yangfei (Felix)]

> -----Original Message-----
> From: Yangfei (Felix)
> Sent: Wednesday, November 13, 2019 10:36 AM
> To: 'Andrew Haley' <aph at redhat.com>; Erik Österlund
> <erik.osterlund at oracle.com>; Andrew Dinn <adinn at redhat.com>;
> aarch64-port-dev at openjdk.java.net
> Cc: hotspot-dev at openjdk.java.net
> Subject: RE: [aarch64-port-dev ] RFR: aarch64: minor improvements of atomic
> operations
> 
> > -----Original Message-----
> > From: Andrew Haley [mailto:aph at redhat.com]
> > Sent: Wednesday, November 13, 2019 3:00 AM
> > To: Erik Österlund <erik.osterlund at oracle.com>; Yangfei (Felix)
> > <felix.yang at huawei.com>; Andrew Dinn <adinn at redhat.com>;
> > aarch64-port-dev at openjdk.java.net
> > Cc: hotspot-dev at openjdk.java.net
> > Subject: Re: [aarch64-port-dev ] RFR: aarch64: minor improvements of
> > atomic operations
> >
> > On 11/12/19 5:38 PM, Erik Österlund wrote:
> > > My hope is that the AArch64 port should use inline assembly as you
> > > suggest,
> > so we can see that the generated code is correct, as we wait for the
> > glorious future where all HotSpot code has been rewritten to work with
> > seq_cst (and we are *not* there now).
> >
> > I don't doubt it. :-)
> >
> > But my arguments about the C++ intrinsics being well-enough defined,
> > at least on AArch64 Linux, have not changed, and I'm not going to argue all
> that again.
> > I'll grant you that there may well be issues on various x86 compilers,
> > but that isn't relevant here.
> 
> Looks like I reignited an old discussion :- )
> 
> > > Now it looks like you have discovered that we sometimes have double
> > > trailing dmb ish, and sometimes lacking leading dmb ish if I am
> > > reading this right. That seems to make the case stronger,
> >
> > Sure, we can use inline asm if there's no other way to do it, but I
> > don't think that's necessary. All we need is to use
> >
> >   T res;
> >   __atomic_exchange(dest, &exchange_value, &res, __ATOMIC_RELEASE);
> >   FULL_MEM_BARRIER;
> >
> 
> When we go the C++ intrinsics way, we should also handle
> Atomic::PlatformCmpxchg.
> When I compile the following function with GCC 4.9.3:
> 
> long foo(long exchange_value, long volatile* dest, long compare_value) {
>   long val = __sync_val_compare_and_swap(dest, compare_value,
> exchange_value);
>   return val;
> }
> 
> I got:
> 
> .L2:
>         ldaxr   x0, [x1]
>         cmp     x0, x2
>         bne     .L3
>         stlxr   w4, x3, [x1]
>         cbnz    w4, .L2
> .L3:
> 
> 
> Proposed patch:
> diff -r 846fee5ea75e
> src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp
> --- a/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp Wed Nov 13
> 10:27:06 2019 +0900
> +++ b/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp Wed Nov
> +++ 13 10:14:58 2019 +0800
> @@ -52,7 +52,7 @@
>                                                       T volatile*
> dest,
> 
> atomic_memory_order order) const {
>    STATIC_ASSERT(byte_size == sizeof(T));
> -  T res = __sync_lock_test_and_set(dest, exchange_value);
> +  T res = __atomic_exchange_n(dest, exchange_value, __ATOMIC_RELEASE);
>    FULL_MEM_BARRIER;
>    return res;
>  }
> @@ -70,7 +70,11 @@
>                                __ATOMIC_RELAXED,
> __ATOMIC_RELAXED);
>      return value;
>    } else {
> -    return __sync_val_compare_and_swap(dest, compare_value,
> exchange_value);
> +    T value = compare_value;
> +    __atomic_compare_exchange(dest, &value, &exchange_value,
> /*weak*/false,
> +                              __ATOMIC_RELEASE,
> __ATOMIC_RELAXED);
> +    FULL_MEM_BARRIER;
> +    return value;
>    }
>  }


Still not strong enough? considering the first of ldxr of the loop may be speculated.  

v2 patch: 
diff -r 846fee5ea75e src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp
--- a/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp Wed Nov 13 10:27:06 2019 +0900
+++ b/src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp Wed Nov 13 16:33:16 2019 +0800
@@ -52,7 +52,7 @@
                                                      T volatile* dest,
                                                      atomic_memory_order order) const {
   STATIC_ASSERT(byte_size == sizeof(T));
-  T res = __sync_lock_test_and_set(dest, exchange_value);
+  T res = __atomic_exchange_n(dest, exchange_value, __ATOMIC_RELEASE);
   FULL_MEM_BARRIER;
   return res;
 }
@@ -70,7 +70,12 @@
                               __ATOMIC_RELAXED, __ATOMIC_RELAXED);
     return value;
   } else {
-    return __sync_val_compare_and_swap(dest, compare_value, exchange_value);
+    T value = compare_value;
+    FULL_MEM_BARRIER;
+    __atomic_compare_exchange(dest, &value, &exchange_value, /*weak*/false,
+                              __ATOMIC_RELAXED, __ATOMIC_RELAXED);
+    FULL_MEM_BARRIER;
+    return value;
   }
 }