RFR: 8186838: Generalize Atomic::inc/dec with templates

[Erik Österlund]

Hi David,

On 2017-09-01 02:49, David Holmes wrote:
> Hi Erik,
>
> Sorry but this one is really losing me.
>
> What is the role of Adjustment ??

Adjustment represents the increment/decrement value as an 
IntegralConstant - your template friend for passing around a constant 
with both a specified type and value in templates. The type of the 
increment/decrement is the type of the destination when the destination 
is an integral type, otherwise if it is a pointer type, the 
increment/decrement type is ptrdiff_t.

> How are inc/dec anything but "using constant" ??

I was also a bit torn on that name (I assume you are referring to 
IncUsingConstant/DecUsingConstant). It was hard to find a name that 
depicted what this platform helper does. I considered calling the helper 
something with immediate in the name because it is really used to embed 
the constant as immediate values in inline assembly today. But then 
again that seemed too specific, as it is not completely obvious platform 
specializations will use it in that way. One might just want to 
specialize this to send it into some compiler Atomic::inc intrinsic for 
example. Do you have any other preferred names? Here are a few possible 
names for IncUsingConstant:

IncUsingScaledConstant
IncUsingAdjustedConstant
IncUsingPlatformHelper

Any favourites?

> Why do we special case jshort??

To be consistent with the special case of Atomic::add on jshort. Do you 
want it removed?

> This is indecipherable to normal people ;-)
>
>  This()->template inc<I, Adjustment>(dest);
>
> For something as trivial as adding or subtracting 1 the template 
> machinations here are just mind boggling!

This uses the CRTP (Curiously Recurring Template Pattern) C++ idiom. The 
idea is to devirtualize a virtual call by passing in the derived type as 
a template parameter to a base class, and then let the base class 
static_cast to the derived class to devirtualize the call. I hope this 
explanation sheds some light on what is going on. The same CRTP idiom 
was used in the Atomic::add implementation in a similar fashion.

I will add some comments describing this in the next round after Coleen 
replies.

Thanks for looking at this.

/Erik

>
> Cheers,
> David
>
> On 31/08/2017 10:45 PM, Erik Österlund wrote:
>> Hi everyone,
>>
>> Bug ID:
>> https://bugs.openjdk.java.net/browse/JDK-8186838
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8186838/webrev.00/
>>
>> The time has come for the next step in generalizing Atomic with 
>> templates. Today I will focus on Atomic::inc/dec.
>>
>> I have tried to mimic the new Kim style that seems to have been 
>> universally accepted. Like Atomic::add and Atomic::cmpxchg, the 
>> structure looks like this:
>>
>> Layer 1) Atomic::inc/dec calls an IncImpl()/DecImpl() function object 
>> that performs some basic type checks.
>> Layer 2) IncImpl/DecImpl calls PlatformInc/PlatformDec that can 
>> define the operation arbitrarily for a given platform. The default 
>> implementation if not specialized for a platform is to call 
>> Atomic::add. So only platforms that want to do something different 
>> than that as an optimization have to provide a specialization.
>> Layer 3) Platforms that decide to specialize PlatformInc/PlatformDec 
>> to be more optimized may inherit from a helper class 
>> IncUsingConstant/DecUsingConstant. This helper helps performing the 
>> necessary computation what the increment/decrement should be after 
>> pointer scaling using CRTP. The PlatformInc/PlatformDec operation 
>> then only needs to define an inc/dec member function, and will then 
>> get all the context information necessary to generate a more 
>> optimized implementation. Easy peasy.
>>
>> It is worth noticing that the generalized Atomic::dec operation 
>> assumes a two's complement integer machine and potentially sends the 
>> unary negative of a potentially unsigned type to Atomic::add. I have 
>> the following comments about this:
>> 1) We already assume in other code that two's complement integers 
>> must be present.
>> 2) A machine that does not have two's complement integers may still 
>> simply provide a specialization that solves the problem in a 
>> different way.
>> 3) The alternative that does not make assumptions about that would 
>> use the good old IntegerTypes::cast_to_signed metaprogramming stuff, 
>> and I seem to recall we thought that was a bit too involved and 
>> complicated.
>> This is the reason why I have chosen to use unary minus on the 
>> potentially unsigned type in the shared helper code that sends the 
>> decrement as an addend to Atomic::add.
>>
>> It would also be nice if somebody with access to PPC and s390 
>> machines could try out the relevant changes there so I do not 
>> accidentally break those platforms. I have blind-coded the addition 
>> of the immediate values passed in to the inline assembly in a way 
>> that I think looks like it should work.
>>
>> Testing:
>> RBT hs-tier3, JPRT --testset hotspot
>>
>> Thanks,
>> /Erik