RFR: 8186476: Generalize Atomic::add with templates

[David Holmes]

Hi Kim,

On 20/08/2017 4:16 PM, Kim Barrett wrote:
> Please review this change to Atomic::add, making it a function
> template with a per-platform underlying implementation.
> 
> This change is similar to 8186166: "Generalize Atomic::cmpxchg with
> templates", with similar goals and taking a similar approach.

So one thing I realized with this change is that the use of the 
templates now obscures the set of "overloads" that are available for a 
given operation. Before you could look at atomic.hpp and see that there 
was no add(jlong, jlong*), but now you only discover the absence of that 
API when you try to use it. It took me a while to figure out the 
machinations for PlatformAdd<8> only existing on 64-bit. :)

Aside: the 1 -> 1u changes are really irksome. I know you explained it 
before but it still seem crazy to me that the compiler can't handle this. :(

> This change deprecates add_ptr.  Followup changes will convert
> existing uses of cmpxchg_ptr and eventually remove it.  Followup
> changes will also update uses of cmpxchg that can be simplified
> because of the generalized API.  Only changes to usage that were
> required by the new implementation have been made so far.
> 
> The add function calls private Atomic::AddImpl, to select among
> several cases, based on the argument types.  Each case performs some
> checking and possibly conversion to reach a standard form of the
> arguments (or fail and report a compile time error), which are then
> passed to private Atomic::PlatformAdd.
> 
> Only safe conversions of the first (add_value) argument are made by
> the AddImpl layer.  add_value must be integral, and is never narrowed.
> dest's value type must be either integral or a pointer type.  When
> both are integral, they must be of the same signedness.  If smaller,
> add_value will be promited to the same size as dest's value type.
> 
> The PlatformAdd class is provided by each platform, and performs
> additional case analysis to generate the appropriate code for the
> platform.  Most PlatformAdd implementations use one of two helper base
> classes, Atomic::FetchAndAdd and Atomic::AddAndFetch.  These provide
> common parts of the implementation in terms of a support function
> provided by the derived PlatformAdd, accessed using CRTP.
> 
> This change involves modifications to all of the existing platform
> implementations of Atomic::add.  I've updated all of the platforms,
> but only tested those supported by Oracle.  I'll need help from the
> various platform maintainers to check and test my changes.
> 
> Changes to files can be categorized as follows:
> 
> [No new metaprogramming tools!]
> 
> Change add to a template:
> atomic.hpp
> 
> Oracle-supported platforms:
> atomic_bsd_x86.hpp - 64bit only
> atomic_linux_arm.hpp
> atomic_linux_x86.hpp
> atomic_solaris_sparc.hpp
> solaris_sparc.il
> atomic_solaris_x86.hpp
> atomic_windows_x86
> 
> Non-Oracle platform changes:
> atomic_aix_ppc.hpp
> atomic_bsd_x86.hpp - 32bit
> atomic_bsd_zero.hpp
> atomic_linux_aarch.hpp
> atomic_linux_ppc.hpp
> atomic_linux_s390.hpp
> atomic_linux_sparc.hpp
> atomic_linux_zero.hpp
> 
> Usage changes required by API change:
> g1CardLiveData.cpp
> g1ConcurrentMark.cpp
> g1HotCardCache.cpp
> g1HotCardCache.hpp
> g1RemSet.cpp
> symbol.cpp
> mallocTracker.hpp
> 
> A few specific items:
> 
> - atomic_linux_arm.hpp
> 
> Neither add variant has "cc" in the clobbers list, even though the
> condition codes are modified.  That seems like a pre-existing bug.
> 
> - atomic_linux_sparc.hpp
> 
> Neither add variant has "cc" in the clobbers list, even though the
> condition codes are modified.  That seems like a pre-existing bug.
> 
> The 32-bit add was using intptr_t as the rv type.  Probably a harmless
> copy-paste mistake.  Now using template parameter type.
> 
> Uses hard-coded machine registers and assumes the inline-asm
> processing assigns the values to the corresponding machine registers,
> even though the given constraints are just generic register requests.
> I don't understand how this can work.
> 
> - atomic_solaris_sparc.hpp
> 
> Atomic::add was implemented using asm code in solaris_sparc.il.  I was
> going to change it to use gcc-style inline-asm, but the linux_sparc
> version wasn't helpful this time (unlike for cmpxchg) (see above).
> Instead, I wrote the CAS-loop in C++, using Atomic::cmpxchg; much
> simpler code, and should be no less efficient, assuming the compiler
> does it's job.

I was getting ready to object to this until I saw that the asm in the 
.il file is already a CAS-loop. :) Makes me wonder whether this version 
should not be in platform independent code ready to be inherited by a 
platform if needed?

> CR:
> https://bugs.openjdk.java.net/browse/JDK-8186476
> 
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8186476/hotspot.00/

Nothing jumped out at me after a few readings :)

Thanks,
David



> Testing:
> Ad hoc hotspot nightly test.
> 
>