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

[Kim Barrett]

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

============================================================================== 
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.

============================================================================== 
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.

============================================================================== 
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.

============================================================================== 
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.

============================================================================== 
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.

==============================================================================