Re: [PATCH v5 16/36] tcg: Add tcg_gen_{non}atomic_cmpxchg_i128

[Philippe Mathieu-Daudé]

On 26/1/23 05:38, Richard Henderson wrote:
> This will allow targets to avoid rolling their own.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/tcg-runtime.h       | 11 +++++
>   include/tcg/tcg-op.h          |  5 +++
>   tcg/tcg-op.c                  | 85 +++++++++++++++++++++++++++++++++++
>   accel/tcg/atomic_common.c.inc | 45 +++++++++++++++++++
>   4 files changed, 146 insertions(+)

> +void tcg_gen_nonatomic_cmpxchg_i128(TCGv_i128 retv, TCGv addr, TCGv_i128 cmpv,
> +                                    TCGv_i128 newv, TCGArg idx, MemOp memop)
> +{
> +    if (TCG_TARGET_REG_BITS == 32) {
> +        /* Inline expansion below is simply too large for 32-bit hosts. */
> +        gen_atomic_cx_i128 gen = ((memop & MO_BSWAP) == MO_LE
> +                                  ? gen_helper_nonatomic_cmpxchgo_le
> +                                  : gen_helper_nonatomic_cmpxchgo_be);
> +        MemOpIdx oi = make_memop_idx(memop, idx);
> +
> +        tcg_debug_assert((memop & MO_SIZE) == MO_128);
> +        tcg_debug_assert((memop & MO_SIGN) == 0);
> +
> +        gen(retv, cpu_env, addr, cmpv, newv, tcg_constant_i32(oi));
> +    } else {
> +        TCGv_i128 oldv = tcg_temp_new_i128();
> +        TCGv_i128 tmpv = tcg_temp_new_i128();
> +        TCGv_i64 t0 = tcg_temp_new_i64();
> +        TCGv_i64 t1 = tcg_temp_new_i64();
> +        TCGv_i64 z = tcg_constant_i64(0);
> +
> +        tcg_gen_qemu_ld_i128(oldv, addr, idx, memop);
> +
> +        /* Compare i128 */
> +        tcg_gen_xor_i64(t0, TCGV128_LOW(oldv), TCGV128_LOW(cmpv));
> +        tcg_gen_xor_i64(t1, TCGV128_HIGH(oldv), TCGV128_HIGH(cmpv));
> +        tcg_gen_or_i64(t0, t0, t1);

Can we skip the OR ...

> +        /* tmpv = equal ? newv : oldv */
> +        tcg_gen_movcond_i64(TCG_COND_EQ, TCGV128_LOW(tmpv), t0, z,
> +                            TCGV128_LOW(newv), TCGV128_LOW(oldv));
> +        tcg_gen_movcond_i64(TCG_COND_EQ, TCGV128_HIGH(tmpv), t0, z,

... and test t1 here to partially update LO/HI? (we could remove t1 and
just use t0 as twice 'xor + movcond' but the code is less readable).

> +                            TCGV128_HIGH(newv), TCGV128_HIGH(oldv));
> +
> +        /* Unconditional writeback. */
> +        tcg_gen_qemu_st_i128(tmpv, addr, idx, memop);
> +        tcg_gen_mov_i128(retv, oldv);
> +
> +        tcg_temp_free_i64(t0);
> +        tcg_temp_free_i64(t1);
> +        tcg_temp_free_i128(tmpv);
> +        tcg_temp_free_i128(oldv);
> +    }
> +}

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>