RFR: 8186166: Generalize Atomic::cmpxchg with templates

[David Holmes]

Hi Kim,

This is an initial partial review. I have to move onto other stuff today 
so will get back to this tomorrow.

On 14/08/2017 12:41 PM, Kim Barrett wrote:
> Please review this change to Atomic::cmpxchg, making it a function
> template with a per-platform underlying implementation.
> 
> This has been broken out from 8184334: "Generalizing Atomic with
> templates", as the originally proposed change there encountered
> various objections.  This change takes a somewhat different approach
> to the same problem.

I assume the intent is not to push this yet, so that Erik has a chance 
to weigh-in when he gets back from vacation? I would not want to see 
unnecessary churn here.

First for the record, and for those who followed the other review 
thread, I have been learning about template metaprogramming under Kim's 
guidance. I can see some uses for it, even though it can be extremely 
difficult to read and interpret. I'm not a Functional Programmer so I 
don't find the style of it particularly appealing, or "natural". But I'm 
no longer opposed to its use in moderation and with good taste - and 
with definite advantage to its use. :)

> One goal is to generalize the API and make it more type-safe,
> eliminating (nearly) all need for casts by callers.  Another is to
> allow more direct per-platform implementations, especially where there
> are appropriate compiler intrinsics available.

Sounds good.

> If all goes well and this change is accepted, it is intended that the
> remainder of Atomic, and later OrderAccess, will be changed in a
> similar manner as one or more followups.

Ok.

> This change to cmpxchg deprecates cmpxchg_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.

Sounds good.

> The cmpxchg function calls private Atomic::CmpxchgImpl, 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::PlatformCmpxchg.  The PlatformCmpxchg class
> is provided by each platform, and performs additional case analysis to
> generate the appropriate code for the platform.

Not sure how many levels of indirection we have here yet ...

> This change involves modifications to all of the existing platform
> implementations of cmpxchg.  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:
> 
> New metaprogramming utilities:
> integerTypes.hpp
> test_integerTypes.cpp
> isRegisteredEnum.hpp
> test_isRegisteredEnum.cpp
> 
> Change cmpxchg 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:
> os_bsd.cpp
> os_solaris.cpp
> aotCodeHeap.cpp
> aotCodeHeap.hpp
> psParallelCompact.hpp
> workgroup.cpp
> oopsHierarchy.hpp
> os.cpp
> 
> Example updates of cmpxchg_ptr to cmpxchg:
> oop.inline.hpp
> bitMap.cpp
> bitMap.inline.hpp
> 
> Some specific issues:
> 
> - For Solaris (both SPARC and x86), we were using helper functions
> defined in corresponding .il files.  Using the cmpxchg_using_stub
> helper didn't work; passing a "defined in .il" function as a template
> parameter didn't work, leading to linker errors.  The ideal solution
> is to use gcc-style inline assembly, which is now supported by Solaris
> Studio.  That change was made for SPARC.  However, making that change
> for x86 ran into related segfaults, so using the .il definition with
> direct conversions for now, until that problem can be resolved.

We already have an issue open for getting rid of the .il files:

https://bugs.openjdk.java.net/browse/JDK-8153076

The template usage probably adds fuel to the argument for doing so.

> - I've added the function template Atomic::condition_store_ptr, which
> is a cmpxchg for pointer types with a NULL compare_value.  That might
> not be the best name for it.

Definitely not the best name for it. :) replace_if_null seems to 
describe it more precisely.

> - The name IntegerTypes was retained from the original RFR for
> JDK-8184334.  However, that name no longer fits the provided
> functionality very well, and I think a new name is needed.  Something
> involving Bitwise, or ValueRepresentation perhaps?  I'm open to
> suggestions.

How about PrimitiveTypes?

> - Issues with casts/conversions provided by IntegerTypes.
> 
> * Conversions between integers and floating point values are
> supported.  It was suggested during the discussion of the RFR for
> JDK-8184334 that this was not needed, as there are no uses of
> atomic/ordered floating point values in Hotspot.  However, I'm told
> VarHandles may include atomic floating point values.

Yes they were added. However at the level of jdk.internal.misc.Unsafe 
all the FP operations are converted to int/long variants using eg 
Float.floatToRawIntBits etc.

But I'm not convinced we need to complicate things by supporting 
floating-point Atomic::cmpxchg in the VM's Atomic API. You can only do 
so by implementing the floatToRawIntBits etc functionality, and there 
are no native mappings for any "atomic" floating-point operation (ie the 
only way to do Atomic::inc on a floating=point value is to use cmpxchg.)

> * Conversions between integers and floating point values are presently
> implemented using the memcpy technique.  While this technique is
> correct, it is known to be quite poorly optimized by some compilers.
> It may be necessary to use the "union trick" for performance reasons,
> even though it is technically undefined behavior.  Hotspot already
> does this elsewhere (and we should merge some of the different
> conversion handling).

More argument for just avoiding this morass if we don't need to go there.

> * In the original RFR for JDK-8184334, conversions between different
> integral types of the same size (along with enums) were performed
> using
> 
>    reinterpret_cast<ToType&>(from_value)
> 
> This was based on discussion from several months ago, referring to
> C++03 3.10/15.  However, re-reading that section in response to
> discussion of the RFR for JDK-8184334, I now think that implementation
> is not supported in many cases needed here, and indeed invokes
> undefined behavior in those cases.  In particular, where 3.10/15 says
> 
>    "- a type that is the signed or unsigned type corresponding to the
>       dynamic type of the object,"
> 
> I interpret this to mean that if the dynamic type of an object is an
> integral type of a given rank, the value may be accessed using an
> lvalue of an integral type of the same rank and the opposite sign.
> (Using the same rank and sign is already permitted, since that's the
> same type.)  I don't think it allows access using an lvalue of an
> integral type with a different rank.  So, for example, a signed int
> value may be accessed via an unsigned int lvalue.  But access via a
> signed long lvalue is not permitted, even if int and long have the
> same size.
> 
> I don't think there is a good way to perform the integral and enum
> conversions we need, without invoking implementation-defined,
> unspecified, or undefined behavior.
> 
> ** The memcpy technique avoids such problems, but as noted above, is
> known to be quite poorly optimized by some compilers.
> 
> ** Using static_cast avoids any undefined behavior, but does have
> cases of implementation-defined or unspecified behavior.  (When
> casting an unsigned value to a signed type, the result may be
> implementation defined; 5.2.9/2 and 4.7/3.  C++11 <type_traits> plus
> the reinterpret_cast to reference seems to provide possibly multi-step
> conversion paths that avoid such qualifications on all of the steps.)
> (When casting an integral value to an enum type, the result may be
> unspecified; 5.2.9/7.  With C++11 <type_traits>, the problem doesn't
> arise so long as the starting value is appropriate, e.g. we're
> reversing an enum to integer conversion.)
> 
> ** Again here, we might consider the "union trick".
> 
> I haven't changed the implementation from the JDK-8184334 RFR, but I'm
> presently leaning toward the "union trick", despite previous strong
> resistance to it.  Right now it seems to me to be the best option in
> practice, despite being disallowed by the standard.  It's really too
> bad the memcpy technique is so poorly supported by some compilers.

I find that all rather dismaying.

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

Looking at the new metaprogramming files first:

src/share/vm/metaprogramming/integerTypes.hpp

With a bit of exertion I can read all this and am 90% confident I 
understand what they do.

src/share/vm/metaprogramming/isRegisteredEnum.hpp

My initial personal preference here is to see registered enum types 
added to this header file rather than as template specializations at the 
point of use. Is that reasonable/feasible, or are there reasons to 
spread this around?

test/native/metaprogramming/test_integerTypes.cpp
test/native/metaprogramming/test_isRegisteredEnum.cpp

Seems minimalist but ok.

Aside: I find it slightly limiting that because these templates involve 
compile-time checks, that we can't actually write negative tests. Seems 
to me that when writing the templates you had to write a test case that 
will fail, to verify the correctness of the template, but you can't 
check-in that testcase. Maybe we need a test harness that can handle 
'tests' that simply fail to compile ... ?

---

That's all for now.

Thanks,
David
-----


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