RFR: 8186166: Generalize Atomic::cmpxchg with templates

[Kim Barrett]

> On Aug 14, 2017, at 1:08 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> This is an initial partial review. I have to move onto other stuff today so will get back to this tomorrow.

Thanks.

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

I had thought it might depend on how quickly this review went.  But
Jesper's request to make jdk/hs temporarily slushy makes a push before
Erik's return seem quite unlikely.

> […]
>> - 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.

That seems like a good name.  I'll wait a bit for other opinions.

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

I'll wait for other opinions.  That name suggests to me that support
for floating point should be included.  And see below.

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

I was about to agree to removal of floating point from here.  Then I
remembered the jmumble_cast suite of functions in
globalDefinitions.hpp.  Erik and I had discussed a unification, with
the jmumble_cast functions built on IntegerTypes.  We can't presently
do that, because of include dependencies, but that's probably fixable.
We see this utility as having application beyond Atomic &etc template
reimplementation.

But clearly the memcpy technique can't be retained, due to the
deficiencies of some compilers.

>> * 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)
>> […]
> 
> I find that all rather dismaying.

No kidding!

> […]
> 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?

That would require moving the complete enum definitions here, far away
from their natural location.  We can't forward declare them here for
registration; forward declaration of enum types doesn't work (doesn't
compile), since the compiler can't determine the underlying type
without knowing the enumerators.  Also, such a move would only be
possible for an enum type defined at namespace scope; class scoped
enums cannot be so moved.

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

When the addition of native testing was being discussed, that was a
feature I mentioned as being needed.  But that doesn't seem to have
happened, and I couldn't find an existing RFE, so I filed a new one:
https://bugs.openjdk.java.net/browse/JDK-8186177
Native testing needs support for compile-fail tests