RFR (M): 8184334: Generalizing Atomic with templates

[Kim Barrett]

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

I converted a few examples in cmpxchg_template_20170806, but there are over 100 left to go, and many of them require caller source changes because they are discarding the relevant type information at the call site in order to conform to the existing API.  For folks other than Andrew who haven’t done the experiment, here’s just one example:

../src/share/vm/oops/method.hpp: In member function 'bool Method::init_method_counters(MethodCounters*)':
../src/share/vm/oops/method.hpp:353:88: error: NULL used in arithmetic [-Werror=pointer-arith]
     return Atomic::cmpxchg_ptr(counters, (volatile void*)&_method_counters, NULL) == NULL;

Note the cast of &_method_counters.  There are lots like that.  I expect most to be easy to fix, and then the information losing cmpxchg_ptr can be removed.  But that's a lot more work than I want to do before getting some feedback on the cmpxchg changes. And I’d actually prefer to separate the two tasks, with cmpxchg_ptr cleanup being a followup incremental set of tasks.  I expect there to be *some* finicky cases that need careful review, and I’d rather not inflict (or have inflicted upon) reviewers with a large and mostly boringly similar set of changes with the occasional lurking tricky bit.

So I think I’m entirely in agreement with Andrew about the target, just not necessarily in the timing of reaching it.

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

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);
  }
};

and maybe an explicit specialization on 2 that errors rather than calling ::cmpxchg if that’s needed?

I intentionally left the unspecialized case undefined, in part to permit platform backends to do that sort of thing.  Though that ought to be a written part of the contract…