Re: [Qemu-devel] [PATCH v6 13/35] tcg: Add atomic helpers

[Emilio G. Cota]

On Tue, Oct 11, 2016 at 14:40:39 -0500, Richard Henderson wrote:
> Add all of cmpxchg, op_fetch, fetch_op, and xchg.
> Handle both endian-ness, and sizes up to 8.
> Handle expanding non-atomically, when emulating in serial.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  Makefile.objs         |   2 +-
>  Makefile.target       |   1 +
>  atomic_template.h     | 173 ++++++++++++++++++++++++++
>  cputlb.c              | 112 ++++++++++++++++-
>  include/qemu/atomic.h |  43 ++++---

I'd have the changes to atomic.h as a separate patch--this one is
pretty big already.

>  tcg-runtime.c         |  49 ++++++--
>  tcg/tcg-op.c          | 328 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tcg/tcg-op.h          |  44 +++++++
>  tcg/tcg-runtime.h     |  75 ++++++++++++
>  tcg/tcg.h             |  53 ++++++++
>  10 files changed, 848 insertions(+), 32 deletions(-)
>  create mode 100644 atomic_template.h
(snip)
> diff --git a/atomic_template.h b/atomic_template.h
> new file mode 100644
> index 0000000..d2c8a08
> --- /dev/null
> +++ b/atomic_template.h
(snip)
> +#if DATA_SIZE == 1
> +# define END
> +#elif defined(HOST_WORDS_BIGENDIAN)
> +# define END  _be
> +#else
> +# define END  _le
> +#endif

It took me a while to figure out that ATOMIC_NAME needs END (ATOMIC_NAME
is defined later in the patch).

It might be clearer to pass it explicitly as a suffix in the macro, as in
#define ATOMIC_NAME(name, suffix) to then have ATOMIC_NAME(cmpxchg, END).

(snip)
> +
> +/* Note that for addition, we need to use a separate cmpxchg loop instead
> +   of bswaps for the reverse-host-endian helpers.  */
> +ABI_TYPE ATOMIC_NAME(fetch_add)(CPUArchState *env, target_ulong addr,
> +                         ABI_TYPE val EXTRA_ARGS)
> +{
> +    DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> +    DATA_TYPE ldo, ldn, ret, sto;
> +
> +    ldo = *haddr;

        ldo = atomic_read(haddr)
would be better here for C11 compliance (or tsan will complain).

> +    while (1) {
> +        ret = BSWAP(ldo);
> +        sto = BSWAP(ret + val);
> +        ldn = atomic_cmpxchg__nocheck(haddr, ldo, sto);
> +        if (ldn == ldo) {
> +            return ret;
> +        }
> +        ldo = ldn;
> +    }
> +}
> +
> +ABI_TYPE ATOMIC_NAME(add_fetch)(CPUArchState *env, target_ulong addr,
> +                         ABI_TYPE val EXTRA_ARGS)
> +{
> +    DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> +    DATA_TYPE ldo, ldn, ret, sto;
> +
> +    ldo = *haddr;

ditto.

No more comments. Have to say though that I really like how with the
tables in tcg-op.c, code ends up looking neat!

                Emilio