RFR (M): 8184334: Generalizing Atomic with templates

[Andrew Haley]

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.

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

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

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