[trans-mem] Beginning of refactoring

[Richard Henderson]

On 05/25/2011 02:10 PM, Torvald Riegel wrote:
> Here's the beginning of a refactoring aimed at being able to merge more
> TM algorithms later on.
> 
> Patch 1: Just a straightfoward rename to make it clear that we're
> dispatching on the level of ABI calls, not internals.

Ok, I guess.  I don't think it matters that much.

> Patch 2: _ITM_dropReferences is not sufficiently defined in the ABI. It
> seems to target some form of open nesting for txnal wrappers, but the
> prose in the ABI specification is unclear. Thus, disable this for now
> (aka fatal runtime error), and expect the related tests to fail. Pick it
> up again once that the ABI has been improved and the use cases are
> clear.

Sure, but please actually delete the code rather than just comment it out.

> Patch 3: The actual change in how ABI calls are dispatched. Also,
> removed method-readonly (broken, will in a similar form reappear in the
> family of globallock-based algorithms), and disabled method-wbetl (needs
> larger refactoring, will be revived/remerged later).

> +CREATE_DISPATCH_FUNCTIONS_T_MEMCPY(M256, GTM::abi_disp()->_, )
> +CREATE_DISPATCH_FUNCTIONS_T_MEMCPY(M64, GTM::abi_disp()->_, )
> +CREATE_DISPATCH_FUNCTIONS_T_MEMCPY(M128, GTM::abi_disp()->_, )
> +CREATE_DISPATCH_FUNCTIONS(GTM::abi_disp()->_, )

What's the point of using "GTM::abi_disp()->_" as a mandatory argument?

Further, that second "M2" argument is universally empty.  What's that?

> +// Creates memcpy/memmove/memset methods.
> +#define CREATE_DISPATCH_METHODS_MEM()  \
> +virtual void _memtransfer(void *dst, const void* src, size_t size,     \
> +bool may_overlap, ls_modifier dst_mod, ls_modifier src_mod)           \
> +{                                                                     \
> +  memtransfer_static(dst, src, size, may_overlap, dst_mod, src_mod);  \
> +}                                                                     \
> +virtual void _memset(void *dst, int c, size_t size, ls_modifier mod)   \
> +{                                                                     \
> +  memset_static(dst, c, size, mod);                                   \
> +}

Why are the memtransfer and memset virtuals distinguished from the statics?
For the patch as written it would seem to be ok to merge them.

> +  if (GTM::abi_dispatch::NONTXNAL == GTM::abi_dispatch::WRITE ||             \
> +      GTM::abi_dispatch::NONTXNAL == GTM::abi_dispatch::READ)                \

Formatting.

> +#define ITM_MEMTRANSFER_DEF(TARGET, M2, NAME, READ, WRITE) \
> +void ITM_REGPARM _ITM_memcpy##NAME(void *dst, const void *src, size_t size)  \
> +{                                                                            \
> +  TARGET##memtransfer##M2 (dst, src, size,                                   \
> +             false, GTM::abi_dispatch::WRITE, GTM::abi_dispatch::READ);      \
> +}                                                                            \
> +void ITM_REGPARM _ITM_memmove##NAME(void *dst, const void *src, size_t size) \
> +{                                                                            \
> +  if (GTM::abi_dispatch::NONTXNAL == GTM::abi_dispatch::WRITE ||             \
> +      GTM::abi_dispatch::NONTXNAL == GTM::abi_dispatch::READ)                \
> +    {                                                                        \
> +      if (((uintptr_t)dst <= (uintptr_t)src ?                                \
> +          (uintptr_t)dst + size > (uintptr_t)src :                           \
> +          (uintptr_t)src + size > (uintptr_t)dst))                           \
> +        GTM::GTM_fatal("_ITM_memmove overlapping and t/nt is not allowed");  \
> +      else                                                                   \
> +        TARGET##memtransfer##M2 (dst, src, size,                             \
> +                false, GTM::abi_dispatch::WRITE, GTM::abi_dispatch::READ);   \
> +    }                                                                        \
> +  TARGET##memtransfer##M2 (dst, src, size,                                   \
> +              true, GTM::abi_dispatch::WRITE, GTM::abi_dispatch::READ);      \
> +}

Ok, I realize we need macros to generate the ABI names both here and in
CREATE_DISPATCH_FUNCTIONS, but can we limit the code within macros to as
little as absolutely possible?

For instance,

  template<abi_dispatch::ls_modifier dst_mod, abi_dispatch::ls_modifier src_mod>
  void abi_memmove(void *dst, const void *src, size_t size)
  {
    if (dst_mod == NONTXNAL || src_mod == NONTXNAL)
      ...
  }

where the actual implementation under macro is limited to a function call.

Missing return/else in there?  Surely not two calls to memtransfer...

> +protected:
> +  /// Transactional load. Will be called from the dispatch methods
> +  /// created below.
> +  template <typename V> static V load(const V* addr, ls_modifier mod)
> +  {
> +    return *addr;
> +  }
> +  /// Transactional store. Will be called from the dispatch methods
> +  /// created below.
> +  template <typename V> static void store(V* addr, const V value,
> +      ls_modifier mod)
> +  {
> +    *addr = value;
> +  }

Why are these here in the base class?  I'm not sure I like having static
functions that are required to be overridden at each instance, wherein if
you forget to implement them the build silently succeeds, but of course
does the wrong thing.

What's with the 3 /// comments?  Can we stick with existing gnu standards?
I'm pretty sure we've got some documentation for at least one of libstdc++,
gold, and the go front end.

> -#define UNUSED		__attribute__((unused))
> -#define ALWAYS_INLINE	__attribute__((always_inline))
> -#ifdef HAVE_ATTRIBUTE_VISIBILITY
> -# define HIDDEN		__attribute__((visibility("hidden")))
> -#else
> -# define HIDDEN
> -#endif
> -
> -#define likely(X)	__builtin_expect((X) != 0, 1)
> -#define unlikely(X)	__builtin_expect((X), 0)
> +#include "common.h"

Why?  We're already in a header that's clearly marked "internal".


r~