RFR (M): 8184334: Generalizing Atomic with templates

[Kim Barrett]

> On Aug 8, 2017, at 9:14 AM, Andrew Haley <aph at redhat.com> wrote:
> 
> On 07/08/17 18:49, Kim Barrett wrote:
>>> On Aug 7, 2017, at 11:23 AM, Andrew Haley <aph at redhat.com> wrote:
>>> 
>>> On 07/08/17 00:32, Kim Barrett wrote:
>>>> I'm looking for feedback on this before I try to carry it any further.
>>> 
>>> I don't like it because it converts pointers to operand types before
>>> calling the back end.
>>> 
>>> For example, in here:
>>> 
>>> intptr_t v = CASPTR(&_LockWord, 0, _LBIT);  // agro ...
>>> 
>>> the type of the operand LockWord is SplitWord.  But the SplitWord *
>>> argument gets converted to void* volatile* when we call this:
>>> 
>>>  inline static void*        cmpxchg_ptr(void*        exchange_value, volatile void*         dest, void*        compare_value, cmpxchg_memory_order order = memory_order_conservative) {
>>>   return cmpxchg(exchange_value,
>>>                  reinterpret_cast<void* volatile*>(dest),
>>>                  compare_value,
>>>                  order);
>>> Here's what I first wrote:
>>> 
>>> I don't see the point of such a type conversion.  We could call
>>> cmpxchg with the actual types of the operands, could we not?  Why is
>>> cmpxchg_ptr even a thing?  We're casting away type information for
>>> no reason that I can see.
>>> 
>>> Couldn't cmpxchg_ptr() be defined as a template function in such a
>>> way that only the back ends that actually need to cast away the
>>> types have to do so?  That is, if the back ends can define
>>> cmpxchg_ptr() themselves without resorting to pointer type
>>> conversion, we should let them so so.
>>> 
>>> But rather than sending that message straight away, I tried it.  And
>>> now I see: the compiler can't get the types right in those cases where
>>> we have mismatched operand types in the call.  Argh.  The only way we
>>> can get method resolution to work is to throw way the pointer type
>>> information and use void* for everything.  At th erisk of being
>>> boring, I repeat what I said before: IMO this is not what we should be
>>> doing in 2017.  We should be looking to the future, and get the types
>>> to match now, at the call site.
>> 
>> Maybe you’ve forgotten this, from Erik’s original RFR email?
>> 
>> "The X_ptr member functions have been deprecated, but are still
>> there and can be used with identical behaviour as they had
>> before. But new code should just use the non-ptr member functions
>> instead.”
> 
> No, I hadn't forgotten, it's because I wrote a version of this patch
> which made the problem go away.  But that did result in a few changes
> at call sites, as discussed.

I recall a discussion about fixing (8-ish) call sites to work with a
requirement that all the types match.  Those were ordinary cmpxchg,
not cmpxchg_ptr.  There are *lots* of cmpxchg_ptr call sites that are
problematic.  Just for starters, there are the 20-25 that pass NULL as
the compare_value.

>> So I think I’m entirely in agreement with Andrew about the target,
>> just not necessarily in the timing of reaching it.
> 
> OK.
> 
>> What’s wrong with
>> 
>> template<size_t /* size */>
>> struct Atomic::PlatformCmpxchg VALUE_OBJ_CLASS_SPEC {
>>  template<typename T>
>>  T operator()(T nv, T volatile* d, T ov, cmpxchg_memory_order order) const {
>>    return ::cmpxchg(nv, d, ov, order);
>>  }
>> };
> 
> Thanks.  That seems to work, but I have no idea why.  :-)

Atomic declares PlatformCmpxchg (and fails to document it's
requirements; I did say this was a prototype...), but does not provide
any definition.  The above definition is unspecialized on the size, so
is applicable for any size value.  Since the body code doesn't seem to
care about the size (or rather, figures out what it needs on its own,
inside ::cmpxchg)...

>> and maybe an explicit specialization on 2 that errors rather than
>> calling ::cmpxchg if that’s needed?
> 
> No, there's no need for that: if anyone uses cmpxchg(short) that'll
> just work.
> 
> I guess I will drop my objection to cmpxchg_ptr() staying as it is,
> because it looks like we have a general improvement on the status quo.
> It certainly seems to work, and everything inlines beautifully.

Great!  Thank you.  

My plan at this point is to focus on finishing cmpxchg, and put just
that (with the associated infrastructure) out for review.  Then circle
back to deal with the other operations, using the new infrastructure,
approach, and any additional lessons learned from cmpxchg.  That
should also make the handoff of remaining work back to Erik go more
smoothly when he comes back from vacation and I start mine.

> -- 
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671