Re: [PATCH 13/18] target/i386: split eflags computation out of gen_compute_eflags

[Richard Henderson]

On 10/14/23 03:01, Paolo Bonzini wrote:
> The new x86 decoder wants to compute EFLAGS before writeback, which
> can be an issue for some instructions such as ARPL.  Extract code
> to compute the EFLAGS without clobbering CC_SRC, in case the ARPL
> memory write causes a fault.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/translate.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index e13bf7df591..2da7c357cdc 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -872,18 +872,20 @@ static void gen_op_update_neg_cc(DisasContext *s)
>       tcg_gen_movi_tl(s->cc_srcT, 0);
>   }
>   
> -/* compute all eflags to cc_src */
> -static void gen_compute_eflags(DisasContext *s)
> +/* compute all eflags to reg */
> +static void gen_mov_eflags(DisasContext *s, TCGv reg)
>   {
>       TCGv zero, dst, src1, src2;
>       int live, dead;
>   
>       if (s->cc_op == CC_OP_EFLAGS) {
> +        if (reg != cpu_cc_src) {
> +            tcg_gen_mov_tl(reg, cpu_cc_src);
> +        }

tcg_gen_mov_tl already takes care of eliding the nop move.

>           return;
>       }
>       if (s->cc_op == CC_OP_CLR) {
> -        tcg_gen_movi_tl(cpu_cc_src, CC_Z | CC_P);
> -        set_cc_op(s, CC_OP_EFLAGS);
> +        tcg_gen_movi_tl(reg, CC_Z | CC_P);
>           return;
>       }
>   
> @@ -909,7 +911,13 @@ static void gen_compute_eflags(DisasContext *s)
>       }
>   
>       gen_update_cc_op(s);
> -    gen_helper_cc_compute_all(cpu_cc_src, dst, src1, src2, cpu_cc_op);
> +    gen_helper_cc_compute_all(reg, dst, src1, src2, cpu_cc_op);
> +}

You don't want to gen_update_cc_op.
I think you want

    TCGv_i32 cc_op = cpu_cc_op;
    if (s->cc_op != CC_OP_DYNAMIC)
        cc_op = tcg_constant_i32(s->cc_op);
    gen_helper_cc_compute_all(..., cc_op);

No write to cpu_cc_op required at all, since...

> +/* compute all eflags to cc_src */
> +static void gen_compute_eflags(DisasContext *s)
> +{
> +    gen_mov_eflags(s, cpu_cc_src);
>       set_cc_op(s, CC_OP_EFLAGS);
>   }

... this will modify it again anyway.


r~