RFR: 8234563: Harmonize parameter order in Atomic

[David Holmes]

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 ??

--

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 ?

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,
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