RFR: 8234563: Harmonize parameter order in Atomic

[David Holmes]

Hi Stefan,

On 22/11/2019 9:14 pm, Stefan Karlsson wrote:
> On 2019-11-22 06:15, David Holmes wrote:
>> Hi Stefan,
>>
>> On 21/11/2019 9:26 pm, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> I'd like to propose a restructuring of the parameter order in Atomic.
>>
>> Okay - consistency is good, especially if it then aligns with external 
>> APIs. Thanks for doing this cleanup!
>>
>>> https://bugs.openjdk.java.net/browse/JDK-8234563
>>>
>>> These are some of the functions used to concurrently write memory 
>>> from HotSpot C++ code:
>>>
>>>   Atomic::store(value, destination);
>>>   OrderAccess::release_store(destination, value)
>>>   OrderAccess::release_store_fence(destination, value)
>>>   Atomic::add(value, destination);
>>>   Atomic::sub(value, destination);
>>>   Atomic::xchg(exchange_value, destination);
>>>   Atomic::cmpxchg(exchange_value, destination, compare_value);
>>>
>>> With the proposed JDK-8234562 change, this would look like:
>>>
>>>   Atomic::store(value, destination);
>>>   Atomic::release_store(destination, value)
>>>   Atomic::release_store_fence(destination, value)
>>>   Atomic::add(value, destination);
>>>   Atomic::sub(value, destination);
>>>   Atomic::xchg(exchange_value, destination);
>>>   Atomic::cmpxchg(exchange_value, destination, compare_value);
>>>
>>> I'd like to propose that we move the destination parameter over to 
>>> the left, and the new value to the right. This would look like this:
>>>
>>>   Atomic::store(destination, value);
>>>   Atomic::release_store(destination, value)
>>>   Atomic::release_store_fence(destination, value)
>>>   Atomic::add(destination, value);
>>>   Atomic::sub(destination, value);
>>>   Atomic::xchg(destination, exchange_value);
>>>   Atomic::cmpxchg(destination, compare_value, exchange_value);
>>
>> I was expecting
>>
>> Atomic::cmpxchg(destination, exchange_value, compare_value);
>>
>> as that would seem more consistent so that we always have:
>> - arg 1 => destination
>> - arg 2 => new (or delta) value
>>
>> but I guess this is consistent with external cmpxchg APIs :(
>>
>>> This would bring the Atomic API more in-line with the order for a 
>>> normal store:
>>>
>>> *destination = value;
>>>
>>> I've split this up into separate patches, each dealing with a 
>>> separate operation:
>>>
>>> Atomic::store:
>>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.01.store/
>>
>> Ok.
>>
>>> Atomic::add:
>>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.02.add/
>>
>> src/hotspot/os_cpu/solaris_x86/solaris_x86_64.il
>>
>> You updated the comments to show the parameters have changed order but 
>> the assembly code is unchanged - won't it need modification too? Or is 
>> it that the true call site for this still uses the original order ??
>>
> 
> Right. I've changed the order for the C++ functions, but left inline 
> assembly that assumes a given parameter order. I didn't want to change 
> that with this patch. Here's where the arguments are swapped:
> 
> // Not using add_using_helper; see comment for cmpxchg.
> template<>
> template<typename D, typename I>
> inline D Atomic::PlatformAdd<4>::add_and_fetch(D volatile* dest, I 
> add_value,
>                                                 atomic_memory_order 
> order) const {
>    STATIC_ASSERT(4 == sizeof(I));
>    STATIC_ASSERT(4 == sizeof(D));
>    return PrimitiveConversions::cast<D>(
>      _Atomic_add(PrimitiveConversions::cast<int32_t>(add_value),
>                  reinterpret_cast<int32_t volatile*>(dest)));
> }

Thanks for pointing that out.

> So, the comment refers to the high-level Atomic::add function, not the 
> _Atomic_add implementation. I agree that it's confusing, and thought 
> about that, but I don't have a good suggestion how to change these 
> comments. If you have a good suggestion on what to changes this to, I 
> can update all these comments.

How about:

   // Support for jint Atomic::add(volatile jint* dest, jint add_value)
   // called via _Atomic_add(jint add_value, volatile jint* dest)

or perhaps

   // Implementation of jint _Atomic_add(jint add_value, volatile jint* 
dest)
   // used by Atomic::add(volatile jint* dest, jint add_value)

>> -- 
>>
>> I was a bit confused about the changes involving 
>> Atomic::add_using_helper:
>>
>> +template<typename Type, typename Fn, typename D, typename I>
>> +inline D Atomic::add_using_helper(Fn fn, D volatile* dest, I 
>> add_value) {
>>     return PrimitiveConversions::cast<D>(
>>       fn(PrimitiveConversions::cast<Type>(add_value),
>>          reinterpret_cast<Type volatile*>(dest)));
>>
>> because you switched the Atomic API parameter order, but the 
>> underlying function being called still has the parameters passed in 
>> the old order. I'm not sure whether this is the right level to stop 
>> the change or whether it should have been pushed all the way down to 
>> e.g. os::atomic_add_func ?
> 
> If I wanted to push this further down I would have to start to change 
> assembly code that relied on a specific order. That's why I chose to 
> stop here.

Understood.

> See:
> add_func_t*          os::atomic_add_func          = 
> os::atomic_add_bootstrap;
> 
> ---
> 
> int32_t os::atomic_add_bootstrap(int32_t add_value, volatile int32_t* 
> dest) {
>    // try to use the stub:
>    add_func_t* func = CAST_TO_FN_PTR(add_func_t*, 
> StubRoutines::atomic_add_entry());
>    if (func != NULL) {
>      os::atomic_add_func = func;
>      return (*func)(add_value, dest);
>    }
>    assert(Threads::number_of_threads() == 0, "for bootstrap only");
>    return (*dest) += add_value;
> }
> 
> ---
> 
> StubRoutines::_atomic_add_entry           = generate_atomic_add();
> 
> ---
>    address generate_atomic_add() {
>      StubCodeMark mark(this, "StubRoutines", "atomic_add");
>      address start = __ pc();
>      __ movl(rax, c_rarg0);
>      __ lock();
>      __ xaddl(Address(c_rarg1, 0), c_rarg0);
>      __ addl(rax, c_rarg0);
>      __ ret(0);
>      return start;
>    }
> 
> I'd prefer to leave that exercise to someone else.

Fair enough :)

Thanks,
David
-----

> 
>>
>> Otherwise seems okay.
>>
>>> Atomic::sub:
>>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.03.sub/
>>
>> Ok.
>>
>>> Atomic::xchg:
>>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.04.xchg/
>>
>> src/hotspot/os_cpu/solaris_x86/solaris_x86_64.il
>>
>> same issue with the assembly as per Atomic::add.
>>
>> ---
>>
>> Same issue/query with xchg_using_helper as add_using_helper.
>>
>>> Atomic::cmpxchg:
>>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.05.cmpxchg/
>>
>> src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
>> src/hotspot/os_cpu/linux_x86/linux_x86_32.s
>> src/hotspot/os_cpu/solaris_x86/solaris_x86_64.il
>>
>> Again queries about the actual assembly code.
> 
> Thanks for reviewing this,
> StefanK
> 
>>
>> Thanks,
>> David
>> -----
>>
>>> All sub-patches combined:
>>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.all/
>>>
>>> The patches applies on-top of the patch for JDK-8234562:
>>>   https://cr.openjdk.java.net/~stefank/8234562/webrev.01/
>>>
>>> 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. However, this is a moving 
>>> target and latest rebase of this has only been run on tier1.
>>>
>>> Thanks,
>>> StefanK