RFR: 8186476: Generalize Atomic::add with templates

[coleen.phillimore at oracle.com]

On 8/28/17 12:33 PM, Erik Osterlund wrote:
> Hi Coleen,
>
>> On 28 Aug 2017, at 14:46, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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?
> Yes. I will file an RFE for that.

Thanks!
>
>> 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.
> I have worked on Atomic::inc/dec in a follow on RFE that I am currently running through testing.
>
>> 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.
> The distinction is whether the platform specialization returns the value before or after the add, i.e., whether whether the returned fetched value corresponds to the old or new value, as indicated by the corresponding name. This is a standard convention used by e.g. GCC intrinsics.

Oh, I see.  Since nothing generally uses the return value, the 
inconsistency doesn't matter?
>
> Unfortunately I have already pushed the change as David and Andrew said I was good to go. I am sorry about that. I will add a short comment in a subsequent inc/dec patch for you.

Okay.
Coleen

> Thanks for the review.
>
> Thanks,
> /Erik
>
>> 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.
>>>
>>>