Re: [Qemu-devel] [PATCH 08/14] i386: do not compute eflags multiple times consecutively

[Richard Henderson <rth@twiddle.net>]

On 10/06/2012 05:30 AM, Paolo Bonzini wrote:
> +static void gen_compute_eflags(DisasContext *s)
>  {
>      if (s->cc_op != CC_OP_DYNAMIC) {
>          gen_op_set_cc_op(s->cc_op);
>      }
> +    if (s->cc_op == CC_OP_EFLAGS) {
> +        return;
>      }
> +    gen_helper_cc_compute_all(cpu_tmp2_i32, cpu_env, cpu_cc_op);
> +    tcg_gen_discard_tl(cpu_cc_dst);
> +    s->cc_op = CC_OP_EFLAGS;
> +    tcg_gen_extu_i32_tl(cpu_cc_src, cpu_tmp2_i32);
>  }

Can we at this point in the series assert that if s->cc_op == CC_OP_EFLAGS,
then cpu_cc_op has also been assigned CC_OP_EFLAGS?  If so, then we can do

    if (s->cc_op == CC_OP_EFLAGS) {
        return;
    }
    if (s->cc_op != CC_OP_DYNAMIC) {
        gen_op_set_cc_op(s->cc_op);
    }
    ...

As-is it would appear that we get redundant assignments to cpu_cc_op when
calling this routine twice in a row.  And with that helper call in between
we won't be able to eliminate the second assignment.

I'll also note that we'd probably get better code if gen_helper_cc_compute_all
took all of cpu_cc_{op,src,dst} as arguments so that it could be CONST+PURE.
With just that changed I think the redundant assignment to cpu_cc_op would
be eliminated.

All that said, I don't see anything wrong with the patch as-is, and probably
these other things I mention would want to be follow-on patches anyway.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~