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

[Erik Österlund]

Hi David,

The shared structure for all operations is the following:

An Atomic::something call creates a SomethingImpl function object that 
performs some basic type checking and then forwards the call straight to 
a PlatformSomething function object. This PlatformSomething object could 
decide to do anything. But to make life easier, it may inherit from a 
shared SomethingHelper function object with CRTP that calls back into 
the PlatformSomething function object to emit inline assembly.

Hope this explanation helps understanding the intended structure of this 
work.

Thanks,
/Erik

On 2017-09-01 12:34, David Holmes wrote:
> Hi Erik,
>
> I just wanted to add that I would expect the cmpxchg, add and inc, 
> Atomic API's to all require similar basic structure for manipulating 
> types/values etc, yet all three seem to have quite different 
> structures that I find very confusing. I'm still at a loss to fathom 
> the CRTP and the hoops we seemingly have to jump through just to add 
> or subtract 1!!!
>
> Cheers,
> David
>
> On 1/09/2017 7:29 PM, Erik Österlund wrote:
>> 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
>>