RFR: 8186476: Generalize Atomic::add with templates

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~kbarrett/8186476/hotspot.00/src/share/vm/runtime/atomic.hpp.udiff.html

Can Atomic::{inc,dec}(volatile jshort* dest) use u2 instead?  And the 
jshort version of Atomic::add use u2 instead of jshort?   Can it be done 
as a follow on change, since I thought we were trying to remove the 
jtypes from this runtime internal code?

I was confused, I thought you'd do Atomic::inc() in this change, but I'm 
in all in favor of limiting changes like this.

What is the distinction between FetchAndAdd and AddAndFetch?   Does one 
assume that the value is in a register and is based on the underlying 
platform implementation?  Can you put a comment in atomic.hpp why one vs 
the other?  These platform inconsistencies are really painful since 
nobody can't test all the platforms.   I don't need to see another 
version of this change, the rest looks ok to me.

Thanks,
Coleen


On 8/20/17 2:16 AM, 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.
>
> 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.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8186476
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8186476/hotspot.00/
>
> Testing:
> Ad hoc hotspot nightly test.
>
>