RFR: 8234562: Move OrderAccess::release_store*/load_acquire to Atomic

[David Holmes]

Hi Stefan,

This generally all seems fine.

I have one concern about header file includes. There are not many 
changes here that change an include of orderAccess.hpp to atomic.hpp. So 
I think we may have missing includes, or at least have very indirect 
include paths. For example compiledMethod.cpp doesn't include 
atomic.hpp, nor does compiledMethod.inline.hpp, but the inline.hpp does 
include orderAccess.hpp which is no longer needed.

Aside: I see some odd looking uses of load_acquire/release_store pairings :(

Minor nits:

src/hotspot/os_cpu/aix_ppc/atomic_aix_ppc.hpp

seems to be an indentation issue:

+    T t = Atomic::load(p);
+	// Use twi-isync for load_acquire (faster than lwsync).
+    __asm__ __volatile__ ("twi 0,%0,0\n isync\n" : : "r" (t) : "memory");
+	return t;

---

src/hotspot/os_cpu/linux_ppc/atomic_linux_ppc.hpp

seems to be an indentation issue:

+    __asm__ __volatile__ ("twi 0,%0,0\n isync\n" : : "r" (t) : "memory");
+	return t;

---

src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp

+// bound calls like release_store go through OrderAccess::load
+// and OrderAccess::store which do volatile memory accesses.

s/OrderAccess/Atomic/

I just realised this used to be technically correct given:

class OrderAccess : private Atomic {

but I personally never realized OrderAccess and Atomic were related this 
way! :)

---

Thanks,
David
-----

On 21/11/2019 8:33 pm, Stefan Karlsson wrote:
> Hi all,
> 
> I'd like to propose that we move release_store, release_store_fence, and 
> load_acquire, from OrderAccess to Atomic.
> 
> https://cr.openjdk.java.net/~stefank/8234562/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8234562
> 
> Atomic already has the relaxed store and load, among other functions for 
> concurrent access, but release_store, release_store_fence, and 
> load_acquire, are located in OrderAccess.
> 
> After this change there's an inconsistency in the order of the 
> parameters in the store functions in the Atomic API:
> 
>   void store(T store_value, volatile D* dest)
>   void release_store(volatile D* dest, T store_value)
>   void release_store_fence(volatile D* dest, T store_value)
> 
> I'd like to address that in a separate RFE, where I propose that we move 
> the dest parameter to the left for all the Atomic functions. See:
> https://bugs.openjdk.java.net/browse/JDK-8234563
> 
> I've tested this patch with tier1-7. I've also built fastdebug on the 
> following configs: aarch64, arm32, ppc64le, s390x, shenandoah, zero, 
> minimal, just to minimize any disruptions.
> 
> Thanks,
> StefanK