RFR (M): 8184334: Generalizing Atomic with templates

[coleen.phillimore at oracle.com]

On 8/7/17 11:23 AM, Andrew Haley 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);

I'm confused.  Is this CASPTR code a reason to not create a template 
layer to cast to the base types, or something to clean up once we have 
it?  Having the void* casts in the caller pushes the undefined behavior 
into all these places.  Having it in the template/atomic layer has it in 
one that can be fixed when we have more modern C++ compilers.


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

I agree.  I think we should fix this code.  It's actually quite 
disturbing because while the literature may have CAS operands in one 
order, all over hotspot we have them in another.  So this should match 
the hotspot code.

Coleen
> Also, and this is a relatively slight objection, I find myself
> defining
>
> template<>
> struct Atomic::PlatformCmpxchg<1> VALUE_OBJ_CLASS_SPEC {
>    template<typename T>
>    T operator()(T exchange_value,
>                 T volatile* dest,
>                 T compare_value,
>                 cmpxchg_memory_order order) const {
>      return ::cmpxchg(exchange_value, dest, compare_value, order);
>    }
> };
>
> for 1, 4, and 8.  I guess that can't be avoided, and in any case it
> would be easy enough to do it with preprocessor macros.
>