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

From: Richard Henderson <richard.henderson@linaro.org>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>, qemu-devel@nongnu.org
Cc: alex.bennee@linaro.org
Subject: Re: [PATCH v5 17/36] tcg: Split out tcg_gen_nonatomic_cmpxchg_i{32,64}
Date: Thu, 26 Jan 2023 20:44:10 -1000	[thread overview]
Message-ID: <79c17cc8-f45d-391f-88db-8d74d32829ef@linaro.org> (raw)
In-Reply-To: <abb025a0-8588-81b3-ddd3-a93b4b66f6f5@linaro.org>

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