RFR (M): 8187977: Generalize Atomic::xchg to use templates

[Erik Österlund]

Hi Kim,

Thanks for looking at this.

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8187977/webrev.00_01/

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8187977/webrev.01/

On 2017-09-28 21:02, Kim Barrett wrote:
>> On Sep 28, 2017, at 7:56 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi all,
>>
>> The time has come to generalize more atomics.
>>
>> I have modelled Atomic::xchg to systematically do what Atomic::cmpxchg
>> did but for xchg.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8187977
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8187977/webrev.00/
>>
>> Testing: mach5 hs-tier3 and JPRT
>>
>> Thanks,
>> /Erik
> ==============================================================================
> src/hotspot/share/runtime/atomic.hpp
>   312   // A default definition is not provided, so specializations must be
>   313   // provided for:
>   314   //   T operator()(T, T volatile*) const
>
> That description is not correct. The default definition of
> PlatformXchg is at line 410. And it makes no sense to talk about
> specializing a function for a class that doesn't exist. This should
> instead be using similar wording to that in the description of
> PlatformCmpxchg.

Fixed.

> ==============================================================================
> src/hotspot/share/runtime/atomic.hpp
>
> There are a lot of similarities between the handling of the
> exchange_value by cmpxchg and xchg, and I expect Atomic::store and
> OrderAccess::release_store and variants to also be similar. (Actually,
> xchg and store may look *very* similar.) It would be nice to think
> about whether there's some refactoring that could be used to reduce
> the amount of code involved. I don't have a specific suggestion yet
> though.  For now, this is just something to think about.

I would not like to mix xchg and load/store implementations in Atomic. 
The reason for that is that I would rather share the implementation 
between Atomic::load/store and OrderAccess::*load/store*, because they 
are even more similar. I have that already in my patch queue.

> ==============================================================================
> src/hotspot/share/compiler/compileBroker.hpp
>   335     Atomic::xchg(jint(shutdown_compilation), &_should_compile_new_jobs);
> [Added jint conversion.]
>
> Pre-existing:
>
> Why is this an Atomic::xchg at all, since the old value isn't used.
> Seems like it could be a release_store.
>
> There also seems to be some data typing problems around
> _should_compile_new_jobs.  Shouldn't that variable be a
> CompilerActivity?  That wouldn't have worked previously, and we're
> probably still not ready for it if the xchg gets changed to a
> release_store, but eventually... so there probably ought to be a bug
> for it.

I will file a bug for that to be reconsidered later on.

> ==============================================================================
> src/hotspot/os_cpu/solaris_x86/atomic_solaris_x86.hpp
>   146 template<>
>   147 template<typename T>
>   148 inline T Atomic::PlatformXchg<8>::operator()(T exchange_value,
>
> Please move this definition up near the PlatformXchg<4> definition,
> e.g. around line 96.

Fixed.

>
> ==============================================================================
> src/hotspot/os_cpu/linux_arm/atomic_linux_arm.hpp
>
> I would prefer the two PlatformXchg specializations be adjacent.  If
> the 4byte specialization was after PlatformAdd<8>, reviewing would
> have been easier, and the different Add would be adjacent and the
> different Xchg would be adjacent.  The only cost is an extra #ifdef
> AARCH64 block around the 8byte Xchg.

Fixed.

> ==============================================================================
> src/hotspot/os_cpu/bsd_zero/atomic_bsd_zero.hpp
>   216   return xchg_using_helper<int>(arm_lock_test_and_set, exchange_value, dest);
>   219   return xchg_using_helper<int>(m68k_lock_test_and_set, exchange_value, dest);
>
> arm/m68k_lock_test_and_set expect their arguments in the other order,
> and need to be updated.  There was a similar change for cmpxchg.
>
> Same problem in atomic_linux_zero.hpp.

Fixed.

Thanks for the review.

/Erik