RFR (M): 8188224: Generalize Atomic::load/store to use templates

[coleen.phillimore at oracle.com]

So this change is becoming more familiar but I think it's because the 
comment is repeated now for cmpxchg, add, and now load and store.   My 
scanning ability is too limited to spot the differences.  I don't like 
the duplicated comments at all.

I don't know if this is possible and not with this change, but I think 
there should be a class platformAtomic.hpp which consolidates these 
comments and moves the platform* stuff out of atomic.hpp, to be included 
or subclassed by atomic.hpp.  Then we can find our desired Atomic::blah 
functions again.   I would like an RFE for this.

Otherwise, I've pattern matched this and it seems correct and am fine 
with checking this in.

http://cr.openjdk.java.net/~eosterlund/8188224/webrev.01/src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp.udiff.html

These changes I really like because now we don't have to go hunting to 
see that atomic::load/store is just *thing.

Thanks!
Coleen

On 10/3/17 8:58 AM, Erik Österlund wrote:
> Hi David,
>
> Thanks for the review.
> The comments have been removed.
>
> New full webrev:
> http://cr.openjdk.java.net/~eosterlund/8188224/webrev.01/
>
> New incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8188224/webrev.00_01/
>
> Thanks,
> /Erik
>
> On 2017-10-03 14:44, David Holmes wrote:
>> Hi Erik,
>>
>> A lot of jumping through hoops just to do a direct load/store in the 
>> bulk of cases - but okay, we're embracing templates.
>>
>> 66   // Atomically store to a location
>> 67   // See comment above about using jlong atomics on 32-bit platforms
>>
>> The comment at #67 and the equivalent one for load can be deleted. 
>> The "comment above" should only be referring to r-m-w atomic ops not 
>> basic load and store. All platforms must have a means to do atomic 
>> load/store of 64-bit due to Java volatile variables (eg by using 
>> floating-point unit on 32-bit) but may not have cmpxchg<8> 
>> capability. (I failed to convince the author of this when those 
>> comments went in. ;-) )
>>
>> Cheers,
>> David
>>
>> On 3/10/2017 10:29 PM, Erik Österlund wrote:
>>> Hi,
>>>
>>> The time has come to generalize Atomic::load/store with templates - 
>>> the last operation to generalize in Atomic.
>>> The design was inspired by Atomic::xchg and uses a similar mechanism 
>>> to validate the passed in arguments. It was also designed with 
>>> coming OrderAccess changes in mind. OrderAccess also contains loads 
>>> and stores that will reuse the LoadImpl and StoreImpl infrastructure 
>>> in Atomic::load/store. (the type checking for what is okay to pass 
>>> in to Atomic::load/store is very much the same for 
>>> OrderAccess::load_acquire/*store*).
>>>
>>> One thing worth mentioning is that the bsd zero port (but notably 
>>> not the linux zero port) had a leading fence for atomic stores of 
>>> jint when #if !defined(ARM) && !defined(M68K) is true without any 
>>> comment describing why. So I took the liberty of removing it. Atomic 
>>> should not have any fencing at all - that is what OrderAccess is 
>>> for. In fact Atomic does not promise any memory ordering semantics 
>>> for loads and stores. Atomic merely provides relaxed accesses that 
>>> are atomic. Worth mentioning nevertheless in case anyone wants to 
>>> keep that jint Atomic::store fence on bsd zero !M68K && !ARM.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8188224
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8188224/webrev.00/
>>>
>>> Testing: JPRT, mach5 hs-tier3
>>>
>>> Thanks,
>>> /Erik
>