Re: [PATCH v5 17/36] tcg: Split out tcg_gen_nonatomic_cmpxchg_i{32,64}

[Richard Henderson]

On 1/26/23 14:53, Philippe Mathieu-Daudé wrote:
> On 26/1/23 05:38, Richard Henderson wrote:
> > Normally this is automatically handled by the CF_PARALLEL checks
> > with in tcg_gen_atomic_cmpxchg_i{32,64}, but x86 has a special
> > case of !PREFIX_LOCK where it always wants the non-atomic version.
> >
> > Split these out so that x86 does not have to roll its own.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   include/tcg/tcg-op.h |   4 ++
> >   tcg/tcg-op.c         | 154 +++++++++++++++++++++++++++----------------
> >   2 files changed, 101 insertions(+), 57 deletions(-)
>
>
> > +void tcg_gen_nonatomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv,
> > +                                   TCGv_i64 newv, TCGArg idx, MemOp memop)
> > +{
> > +    TCGv_i64 t1, t2;
> > +
>
> This block from here ...
>
> > +    if (TCG_TARGET_REG_BITS == 32 && (memop & MO_SIZE) < MO_64) {
> > +        tcg_gen_nonatomic_cmpxchg_i32(TCGV_LOW(retv), addr, TCGV_LOW(cmpv),
> > +                                      TCGV_LOW(newv), idx, memop);
> > +        if (memop & MO_SIGN) {
> > +            tcg_gen_sari_i32(TCGV_HIGH(retv), TCGV_LOW(retv), 31);
> > +        } else {
> > +            tcg_gen_movi_i32(TCGV_HIGH(retv), 0);
> > +        }
> > +        return;
> > +    }
>
> ... to here,
>
> > +    t1 = tcg_temp_new_i64();
> > +    t2 = tcg_temp_new_i64();
> > +
> > +    tcg_gen_ext_i64(t2, cmpv, memop & MO_SIZE);
> > +
> > +    tcg_gen_qemu_ld_i64(t1, addr, idx, memop & ~MO_SIGN);
> > +    tcg_gen_movcond_i64(TCG_COND_EQ, t2, t1, t2, newv, t1);
> > +    tcg_gen_qemu_st_i64(t2, addr, idx, memop);
> > +    tcg_temp_free_i64(t2);
> > +
> > +    if (memop & MO_SIGN) {
> > +        tcg_gen_ext_i64(retv, t1, memop);
> > +    } else {
> > +        tcg_gen_mov_i64(retv, t1);
> > +    }
> > +    tcg_temp_free_i64(t1);
> >   }
> >   void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv,
> >                                   TCGv_i64 newv, TCGArg idx, MemOp memop)
> >   {
> > -    memop = tcg_canonicalize_memop(memop, 1, 0);
> > -
> >       if (!(tcg_ctx->gen_tb->cflags & CF_PARALLEL)) {
> > -        TCGv_i64 t1 = tcg_temp_new_i64();
> > -        TCGv_i64 t2 = tcg_temp_new_i64();
> > +        tcg_gen_nonatomic_cmpxchg_i64(retv, addr, cmpv, newv, idx, memop);
> > +        return;
> > +    }
> > -        tcg_gen_ext_i64(t2, cmpv, memop & MO_SIZE);
> > -
> > -        tcg_gen_qemu_ld_i64(t1, addr, idx, memop & ~MO_SIGN);
> > -        tcg_gen_movcond_i64(TCG_COND_EQ, t2, t1, t2, newv, t1);
> > -        tcg_gen_qemu_st_i64(t2, addr, idx, memop);
> > -        tcg_temp_free_i64(t2);
> > -
> > -        if (memop & MO_SIGN) {
> > -            tcg_gen_ext_i64(retv, t1, memop);
> > -        } else {
> > -            tcg_gen_mov_i64(retv, t1);
> > -        }
> > -        tcg_temp_free_i64(t1);
> > -    } else if ((memop & MO_SIZE) == MO_64) {
> > -#ifdef CONFIG_ATOMIC64
> > +    if ((memop & MO_SIZE) == MO_64) {
> >           gen_atomic_cx_i64 gen;
> > -        MemOpIdx oi;
> > +        memop = tcg_canonicalize_memop(memop, 1, 0);
> >           gen = table_cmpxchg[memop & (MO_SIZE | MO_BSWAP)];
> > -        tcg_debug_assert(gen != NULL);
> > +        if (gen) {
> > +            MemOpIdx oi = make_memop_idx(memop, idx);
> > +            gen(retv, cpu_env, addr, cmpv, newv, tcg_constant_i32(oi));
> > +            return;
> > +        }
> > -        oi = make_memop_idx(memop, idx);
> > -        gen(retv, cpu_env, addr, cmpv, newv, tcg_constant_i32(oi));
> > -#else
> >           gen_helper_exit_atomic(cpu_env);
> > -        /* Produce a result, so that we have a well-formed opcode stream
> > -           with respect to uses of the result in the (dead) code following.  */
> > +
> > +        /*
> > +         * Produce a result for a well-formed opcode stream.  This satisfies
> > +         * liveness for set before used, which happens before this dead code
> > +         * is removed.
> > +         */
> >           tcg_gen_movi_i64(retv, 0);
> > -#endif /* CONFIG_ATOMIC64 */
> > +        return;
> > +    }
>
> and this one here:
> > +    if (TCG_TARGET_REG_BITS == 32) {
> > +        tcg_gen_atomic_cmpxchg_i32(TCGV_LOW(retv), addr, TCGV_LOW(cmpv),
> > +                                   TCGV_LOW(newv), idx, memop);
> > +        if (memop & MO_SIGN) {
> > +            tcg_gen_sari_i32(TCGV_HIGH(retv), TCGV_LOW(retv), 31);
> > +        } else {
> > +            tcg_gen_movi_i32(TCGV_HIGH(retv), 0);
> > +        }
>
> belong to a subsequent patch IMO.

No.  That code is in there now, and needs to be there for correctness.

It gets duplicated in the exposure of the non-atomic code path.


r~


 Otherwise LGTM.
> >       } else {
> >           TCGv_i32 c32 = tcg_temp_new_i32();
> >           TCGv_i32 n32 = tcg_temp_new_i32();