Re: [PULL 13/25] target/i386: convert CMPXCHG8B/CMPXCHG16B to new decoder

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi,

On 15/10/24 11:16, Paolo Bonzini wrote:
> The gen_cmpxchg8b and gen_cmpxchg16b functions even have the correct
> prototype already; the only thing that needs to be done is removing the
> gen_lea_modrm() call.
> 
> This moves the last LOCK-enabled instructions to the new decoder.  It is
> now possible to assume that gen_multi0F is called only after checking
> that PREFIX_LOCK was not specified.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/decode-new.h     |   2 +
>   target/i386/tcg/translate.c      | 121 +------------------------------
>   target/i386/tcg/decode-new.c.inc |  34 ++++++---
>   target/i386/tcg/emit.c.inc       |  96 ++++++++++++++++++++++++
>   4 files changed, 124 insertions(+), 129 deletions(-)


> +static void gen_CMPXCHG8B(DisasContext *s, X86DecodedInsn *decode)
> +{
> +    TCGv_i64 cmp, val, old;
> +    TCGv Z;
> +
> +    cmp = tcg_temp_new_i64();
> +    val = tcg_temp_new_i64();
> +    old = tcg_temp_new_i64();
> +
> +    /* Construct the comparison values from the register pair. */
> +    tcg_gen_concat_tl_i64(cmp, cpu_regs[R_EAX], cpu_regs[R_EDX]);
> +    tcg_gen_concat_tl_i64(val, cpu_regs[R_EBX], cpu_regs[R_ECX]);
> +
> +    /* Only require atomic with LOCK; non-parallel handled in generator. */
> +    if (s->prefix & PREFIX_LOCK) {
> +        tcg_gen_atomic_cmpxchg_i64(old, s->A0, cmp, val, s->mem_index, MO_TEUQ);
> +    } else {
> +        tcg_gen_nonatomic_cmpxchg_i64(old, s->A0, cmp, val,
> +                                      s->mem_index, MO_TEUQ);
> +    }
> +
> +    /* Set tmp0 to match the required value of Z. */
> +    tcg_gen_setcond_i64(TCG_COND_EQ, cmp, old, cmp);
> +    Z = tcg_temp_new();
> +    tcg_gen_trunc_i64_tl(Z, cmp);
> +
> +    /*
> +     * Extract the result values for the register pair.
> +     * For 32-bit, we may do this unconditionally, because on success (Z=1),
> +     * the old value matches the previous value in EDX:EAX.  For x86_64,
> +     * the store must be conditional, because we must leave the source
> +     * registers unchanged on success, and zero-extend the writeback
> +     * on failure (Z=0).
> +     */
> +    if (TARGET_LONG_BITS == 32) {
> +        tcg_gen_extr_i64_tl(cpu_regs[R_EAX], cpu_regs[R_EDX], old);
> +    } else {
> +        TCGv zero = tcg_constant_tl(0);
> +
> +        tcg_gen_extr_i64_tl(s->T0, s->T1, old);
> +        tcg_gen_movcond_tl(TCG_COND_EQ, cpu_regs[R_EAX], Z, zero,
> +                           s->T0, cpu_regs[R_EAX]);
> +        tcg_gen_movcond_tl(TCG_COND_EQ, cpu_regs[R_EDX], Z, zero,
> +                           s->T1, cpu_regs[R_EDX]);
> +    }
> +
> +    /* Update Z. */
> +    gen_compute_eflags(s);
> +    tcg_gen_deposit_tl(cpu_cc_src, cpu_cc_src, Z, ctz32(CC_Z), 1);
> +}

On s390x the cdrom-test generates:

tcg/s390x/tcg-target.c.inc:1284:tgen_cmp2: code should not be reached