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

[coleen.phillimore at oracle.com]

On 10/4/17 8:08 AM, coleen.phillimore at oracle.com wrote:
>
> 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.
^ long (> 1 line)
>
> 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
>>
>