RFR: 8186476: Generalize Atomic::add with templates

[David Holmes]

On 29/08/2017 5:33 AM, coleen.phillimore at oracle.com wrote:
> On 8/28/17 12:33 PM, Erik Osterlund wrote:
>>> 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?

The Atomic::add API specifies that the returned value is the updated 
value. I must confess I'm now a bit confused here as it seems to me that 
PlatformAdd should always be a "AddAndFetch" to ensure the correct 
semantics. ???

David
-----


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