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

[coleen.phillimore at oracle.com]

On 10/4/17 9:06 AM, Erik Österlund wrote:
> Hi Coleen,
>
> On 2017-10-04 14:08, 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.
>>
>> 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.
>
> I see what you are saying. When you think about it, is almost as if we 
> want the comments themselves to be template expanded for each 
> operation. (joking)

LOL, I almost wrote this :)
> I will file an RFE for this.

Thanks!
Coleen

>
>
>> 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.
>
> Thank you for the review!
>
> /Erik
>
>>
>> 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
>>>
>>
>