[11/14] i386: convert gen_compute_eflags_c to TCG

Do the switch at translation time, converting the helper templates to
TCG opcodes.  In some cases CF can be computed with a single setcond,
though others it may require a little more work.

In the CC_OP_DYNAMIC case, compute the whole EFLAGS, same as for ZF/SF/PF.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cc_helper.c          | 118 ---------------------------------
 target-i386/cc_helper_template.h |  76 ----------------------
 target-i386/helper.h             |   1 -
 target-i386/translate.c          | 137 +++++++++++++++++++++++++++++++++++----
 4 file modificati, 124 inserzioni(+), 208 rimozioni(-)

On Sat, Oct 6, 2012 at 12:30 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Do the switch at translation time, converting the helper templates to
> TCG opcodes.  In some cases CF can be computed with a single setcond,
> though others it may require a little more work.
>
> In the CC_OP_DYNAMIC case, compute the whole EFLAGS, same as for ZF/SF/PF.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-i386/cc_helper.c          | 118 ---------------------------------
>  target-i386/cc_helper_template.h |  76 ----------------------
>  target-i386/helper.h             |   1 -
>  target-i386/translate.c          | 137 +++++++++++++++++++++++++++++++++++----
>  4 file modificati, 124 inserzioni(+), 208 rimozioni(-)
>
> diff --git a/target-i386/cc_helper.c b/target-i386/cc_helper.c
> index 9422003..214d715 100644
> --- a/target-i386/cc_helper.c
> +++ b/target-i386/cc_helper.c
> @@ -80,11 +80,6 @@ static int compute_all_eflags(CPUX86State *env)
>      return CC_SRC;
>  }
>
> -static int compute_c_eflags(CPUX86State *env)
> -{
> -    return CC_SRC & CC_C;
> -}
> -
>  uint32_t helper_cc_compute_all(CPUX86State *env, int op)
>  {
>      switch (op) {
> @@ -203,119 +198,6 @@ uint32_t cpu_cc_compute_all(CPUX86State *env, int op)
>      return helper_cc_compute_all(env, op);
>  }
>
> -uint32_t helper_cc_compute_c(CPUX86State *env, int op)
> -{
> -    switch (op) {
> -    default: /* should never happen */
> -        return 0;
> -
> -    case CC_OP_EFLAGS:
> -        return compute_c_eflags(env);
> -
> -    case CC_OP_MULB:
> -        return compute_c_mull(env);
> -    case CC_OP_MULW:
> -        return compute_c_mull(env);
> -    case CC_OP_MULL:
> -        return compute_c_mull(env);
> -
> -    case CC_OP_ADDB:
> -        return compute_c_addb(env);
> -    case CC_OP_ADDW:
> -        return compute_c_addw(env);
> -    case CC_OP_ADDL:
> -        return compute_c_addl(env);
> -
> -    case CC_OP_ADCB:
> -        return compute_c_adcb(env);
> -    case CC_OP_ADCW:
> -        return compute_c_adcw(env);
> -    case CC_OP_ADCL:
> -        return compute_c_adcl(env);
> -
> -    case CC_OP_SUBB:
> -        return compute_c_subb(env);
> -    case CC_OP_SUBW:
> -        return compute_c_subw(env);
> -    case CC_OP_SUBL:
> -        return compute_c_subl(env);
> -
> -    case CC_OP_SBBB:
> -        return compute_c_sbbb(env);
> -    case CC_OP_SBBW:
> -        return compute_c_sbbw(env);
> -    case CC_OP_SBBL:
> -        return compute_c_sbbl(env);
> -
> -    case CC_OP_LOGICB:
> -        return compute_c_logicb();
> -    case CC_OP_LOGICW:
> -        return compute_c_logicw();
> -    case CC_OP_LOGICL:
> -        return compute_c_logicl();
> -
> -    case CC_OP_INCB:
> -        return compute_c_incl(env);
> -    case CC_OP_INCW:
> -        return compute_c_incl(env);
> -    case CC_OP_INCL:
> -        return compute_c_incl(env);
> -
> -    case CC_OP_DECB:
> -        return compute_c_incl(env);
> -    case CC_OP_DECW:
> -        return compute_c_incl(env);
> -    case CC_OP_DECL:
> -        return compute_c_incl(env);
> -
> -    case CC_OP_SHLB:
> -        return compute_c_shlb(env);
> -    case CC_OP_SHLW:
> -        return compute_c_shlw(env);
> -    case CC_OP_SHLL:
> -        return compute_c_shll(env);
> -
> -    case CC_OP_SARB:
> -        return compute_c_sarl(env);
> -    case CC_OP_SARW:
> -        return compute_c_sarl(env);
> -    case CC_OP_SARL:
> -        return compute_c_sarl(env);
> -
> -#ifdef TARGET_X86_64
> -    case CC_OP_MULQ:
> -        return compute_c_mull(env);
> -
> -    case CC_OP_ADDQ:
> -        return compute_c_addq(env);
> -
> -    case CC_OP_ADCQ:
> -        return compute_c_adcq(env);
> -
> -    case CC_OP_SUBQ:
> -        return compute_c_subq(env);
> -
> -    case CC_OP_SBBQ:
> -        return compute_c_sbbq(env);
> -
> -    case CC_OP_LOGICQ:
> -        return compute_c_logicq();
> -
> -    case CC_OP_INCQ:
> -        return compute_c_incl(env);
> -
> -    case CC_OP_DECQ:
> -        return compute_c_incl(env);
> -
> -    case CC_OP_SHLQ:
> -        return compute_c_shlq(env);
> -
> -    case CC_OP_SARQ:
> -        return compute_c_sarl(env);
> -#endif
> -    }
> -}
> -
>  void helper_write_eflags(CPUX86State *env, target_ulong t0,
>                           uint32_t update_mask)
>  {
> diff --git a/target-i386/cc_helper_template.h b/target-i386/cc_helper_template.h
> index 1f94e11..951ceaf 100644
> --- a/target-i386/cc_helper_template.h
> +++ b/target-i386/cc_helper_template.h
> @@ -58,16 +58,6 @@ static int glue(compute_all_add, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -static int glue(compute_c_add, SUFFIX)(CPUX86State *env)
> -{
> -    int cf;
> -    target_long src1;
> -
> -    src1 = CC_SRC;
> -    cf = (DATA_TYPE)CC_DST < (DATA_TYPE)src1;
> -    return cf;
> -}
> -
>  static int glue(compute_all_adc, SUFFIX)(CPUX86State *env)
>  {
>      int cf, pf, af, zf, sf, of;
> @@ -84,16 +74,6 @@ static int glue(compute_all_adc, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -static int glue(compute_c_adc, SUFFIX)(CPUX86State *env)
> -{
> -    int cf;
> -    target_long src1;
> -
> -    src1 = CC_SRC;
> -    cf = (DATA_TYPE)CC_DST <= (DATA_TYPE)src1;
> -    return cf;
> -}
> -
>  static int glue(compute_all_sub, SUFFIX)(CPUX86State *env)
>  {
>      int cf, pf, af, zf, sf, of;
> @@ -110,17 +90,6 @@ static int glue(compute_all_sub, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -static int glue(compute_c_sub, SUFFIX)(CPUX86State *env)
> -{
> -    int cf;
> -    target_long src1, src2;
> -
> -    src1 = CC_DST + CC_SRC;
> -    src2 = CC_SRC;
> -    cf = (DATA_TYPE)src1 < (DATA_TYPE)src2;
> -    return cf;
> -}
> -
>  static int glue(compute_all_sbb, SUFFIX)(CPUX86State *env)
>  {
>      int cf, pf, af, zf, sf, of;
> @@ -137,17 +106,6 @@ static int glue(compute_all_sbb, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -static int glue(compute_c_sbb, SUFFIX)(CPUX86State *env)
> -{
> -    int cf;
> -    target_long src1, src2;
> -
> -    src1 = CC_DST + CC_SRC + 1;
> -    src2 = CC_SRC;
> -    cf = (DATA_TYPE)src1 <= (DATA_TYPE)src2;
> -    return cf;
> -}
> -
>  static int glue(compute_all_logic, SUFFIX)(CPUX86State *env)
>  {
>      int cf, pf, af, zf, sf, of;
> @@ -161,11 +119,6 @@ static int glue(compute_all_logic, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -static int glue(compute_c_logic, SUFFIX)(void)
> -{
> -    return 0;
> -}
> -
>  static int glue(compute_all_inc, SUFFIX)(CPUX86State *env)
>  {
>      int cf, pf, af, zf, sf, of;
> @@ -182,13 +135,6 @@ static int glue(compute_all_inc, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -#if DATA_BITS == 32
> -static int glue(compute_c_inc, SUFFIX)(CPUX86State *env)
> -{
> -    return CC_SRC;
> -}
> -#endif
> -
>  static int glue(compute_all_dec, SUFFIX)(CPUX86State *env)
>  {
>      int cf, pf, af, zf, sf, of;
> @@ -219,18 +165,6 @@ static int glue(compute_all_shl, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -static int glue(compute_c_shl, SUFFIX)(CPUX86State *env)
> -{
> -    return (CC_SRC >> (DATA_BITS - 1)) & CC_C;
> -}
> -
> -#if DATA_BITS == 32
> -static int glue(compute_c_sar, SUFFIX)(CPUX86State *env)
> -{
> -    return CC_SRC & 1;
> -}
> -#endif
> -
>  static int glue(compute_all_sar, SUFFIX)(CPUX86State *env)
>  {
>      int cf, pf, af, zf, sf, of;
> @@ -245,16 +179,6 @@ static int glue(compute_all_sar, SUFFIX)(CPUX86State *env)
>      return cf | pf | af | zf | sf | of;
>  }
>
> -#if DATA_BITS == 32
> -static int glue(compute_c_mul, SUFFIX)(CPUX86State *env)
> -{
> -    int cf;
> -
> -    cf = (CC_SRC != 0);
> -    return cf;
> -}
> -#endif
> -
>  /* NOTE: we compute the flags like the P4. On olders CPUs, only OF and
>     CF are modified and it is slower to do that. */
>  static int glue(compute_all_mul, SUFFIX)(CPUX86State *env)
> diff --git a/target-i386/helper.h b/target-i386/helper.h
> index 93850ce..2f54753 100644
> --- a/target-i386/helper.h
> +++ b/target-i386/helper.h
> @@ -1,7 +1,6 @@
>  #include "def-helper.h"
>
>  DEF_HELPER_FLAGS_2(cc_compute_all, TCG_CALL_PURE, i32, env, int)
> -DEF_HELPER_FLAGS_2(cc_compute_c, TCG_CALL_PURE, i32, env, int)
>
>  DEF_HELPER_0(lock, void)
>  DEF_HELPER_0(unlock, void)
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index abcd944..4561c9d 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -823,19 +823,6 @@ static void gen_op_update_neg_cc(void)
>      tcg_gen_mov_tl(cpu_cc_dst, cpu_T[0]);
>  }
>
> -/* compute eflags.C to reg */
> -static void gen_compute_eflags_c(DisasContext *s, TCGv reg, bool inv)
> -{
> -    if (s->cc_op != CC_OP_DYNAMIC) {
> -        gen_op_set_cc_op(s->cc_op);
> -    }
> -    gen_helper_cc_compute_c(cpu_tmp2_i32, cpu_env, cpu_cc_op);
> -    tcg_gen_extu_i32_tl(reg, cpu_tmp2_i32);
> -    if (inv) {
> -        tcg_gen_xori_tl(reg, reg, 1);
> -    }
> -}
> -
>  /* compute all eflags to cc_src */
>  static void gen_compute_eflags(DisasContext *s)
>  {
> @@ -851,6 +838,130 @@ static void gen_compute_eflags(DisasContext *s)
>      tcg_gen_extu_i32_tl(cpu_cc_src, cpu_tmp2_i32);
>  }
>
> +/* compute eflags.C to reg */
> +static void gen_compute_eflags_c(DisasContext *s, TCGv reg, bool inv)
> +{
> +    int t0, t1, size;
> +
> +    if (s->cc_op == CC_OP_DYNAMIC) {
> +        gen_compute_eflags(s);
> +    }
> +    switch(s->cc_op) {
> +    case CC_OP_SUBB:
> +    case CC_OP_SUBW:
> +    case CC_OP_SUBL:
> +    case CC_OP_SUBQ:
> +        /* (DATA_TYPE)(CC_DST + CC_SRC) < (DATA_TYPE)CC_SRC */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;
> +        t1 = gen_ext_tl(cpu_tmp0, cpu_cc_src, size, false);
> +        if (t1 == reg && reg == cpu_cc_src) {
> +            tcg_gen_mov_tl(cpu_tmp0, cpu_cc_src);
> +            t1 = cpu_tmp0;
> +        }
> +
> +        tcg_gen_add_tl(reg, cpu_cc_dst, cpu_cc_src);
> +        gen_extu(size, reg);
> +        t0 = reg;
> +        goto add_sub;
> +
> +    case CC_OP_ADDB:
> +    case CC_OP_ADDW:
> +    case CC_OP_ADDL:
> +    case CC_OP_ADDQ:
> +        /* (DATA_TYPE)CC_DST < (DATA_TYPE)CC_SRC */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;
> +        t1 = gen_ext_tl(cpu_tmp0, cpu_cc_src, size, false);
> +        t0 = gen_ext_tl(reg, cpu_cc_dst, size, false);
> +    add_sub:
> +        tcg_gen_setcond_tl(inv ? TCG_COND_GEU : TCG_COND_LTU, reg, t0, t1);
> +        return;

It's a tad confusing that 'return' and 'break' are used in a seemingly
random fashion. How about repeating the last few lines for 'break'
cases, or setting 'inv' to false in 'return' cases?

Otherwise the patch looks correct.

> +
> +    case CC_OP_SBBB:
> +    case CC_OP_SBBW:
> +    case CC_OP_SBBL:
> +    case CC_OP_SBBQ:
> +        /* (DATA_TYPE)(CC_DST + CC_SRC + 1) <= (DATA_TYPE)CC_SRC */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;
> +        t1 = gen_ext_tl(cpu_tmp0, cpu_cc_src, size, false);
> +        if (t1 == reg && reg == cpu_cc_src) {
> +            tcg_gen_mov_tl(cpu_tmp0, cpu_cc_src);
> +            t1 = cpu_tmp0;
> +        }
> +
> +        tcg_gen_add_tl(reg, cpu_cc_dst, cpu_cc_src);
> +        tcg_gen_addi_tl(reg, reg, 1);
> +        gen_extu(size, reg);
> +        t0 = reg;
> +        goto adc_sbb;
> +
> +    case CC_OP_ADCB:
> +    case CC_OP_ADCW:
> +    case CC_OP_ADCL:
> +    case CC_OP_ADCQ:
> +        /* (DATA_TYPE)CC_DST <= (DATA_TYPE)CC_SRC */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;
> +        t1 = gen_ext_tl(cpu_tmp0, cpu_cc_src, size, false);
> +        t0 = gen_ext_tl(reg, cpu_cc_dst, size, false);
> +    adc_sbb:
> +        tcg_gen_setcond_tl(inv ? TCG_COND_GTU : TCG_COND_LEU, reg, t0, t1);
> +        return;
> +
> +    case CC_OP_LOGICB:
> +    case CC_OP_LOGICW:
> +    case CC_OP_LOGICL:
> +    case CC_OP_LOGICQ:
> +        tcg_gen_movi_tl(reg, 0);
> +        break;
> +
> +    case CC_OP_INCB:
> +    case CC_OP_INCW:
> +    case CC_OP_INCL:
> +    case CC_OP_INCQ:
> +    case CC_OP_DECB:
> +    case CC_OP_DECW:
> +    case CC_OP_DECL:
> +    case CC_OP_DECQ:
> +        if (inv) {
> +            tcg_gen_xori_tl(reg, cpu_cc_src, 1);
> +        } else {
> +            tcg_gen_mov_tl(reg, cpu_cc_src);
> +        }
> +        return;
> +
> +    case CC_OP_SHLB:
> +    case CC_OP_SHLW:
> +    case CC_OP_SHLL:
> +    case CC_OP_SHLQ:
> +        /* (CC_SRC >> (DATA_BITS - 1)) & 1 */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;
> +        tcg_gen_shri_tl(reg, cpu_cc_src, (8 << size) - 1);
> +        tcg_gen_andi_tl(reg, reg, 1);
> +        break;
> +
> +    case CC_OP_MULB:
> +    case CC_OP_MULW:
> +    case CC_OP_MULL:
> +    case CC_OP_MULQ:
> +        tcg_gen_setcondi_tl(inv ? TCG_COND_EQ : TCG_COND_NE, reg, cpu_cc_src, 0);
> +        return;
> +
> +    case CC_OP_SARB:
> +    case CC_OP_SARW:
> +    case CC_OP_SARL:
> +    case CC_OP_SARQ:
> +    case CC_OP_EFLAGS:
> +        /* CC_SRC & 1 */
> +        tcg_gen_andi_tl(reg, cpu_cc_src, 1);
> +        break;
> +
> +    default:
> +        abort();
> +    }
> +    if (inv) {
> +        tcg_gen_xori_tl(reg, reg, 1);
> +    }
> +}
> +
>  /* compute eflags.P to reg */
>  static void gen_compute_eflags_p(DisasContext *s, TCGv reg)
>  {
> --
> 1.7.12.1
>
>
>

On 10/06/2012 05:30 AM, Paolo Bonzini wrote:
> +    case CC_OP_SUBB:
> +    case CC_OP_SUBW:
> +    case CC_OP_SUBL:
> +    case CC_OP_SUBQ:
> +        /* (DATA_TYPE)(CC_DST + CC_SRC) < (DATA_TYPE)CC_SRC */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;

I guess the & 3 makes the result "just so happen" to be right,
but I think the code would be more readable with "- SUBB" there.
And the other cases of the same pattern below.

> +    case CC_OP_SBBB:
> +    case CC_OP_SBBW:
> +    case CC_OP_SBBL:
> +    case CC_OP_SBBQ:
> +        /* (DATA_TYPE)(CC_DST + CC_SRC + 1) <= (DATA_TYPE)CC_SRC */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;
> +        t1 = gen_ext_tl(cpu_tmp0, cpu_cc_src, size, false);
> +        if (t1 == reg && reg == cpu_cc_src) {
> +            tcg_gen_mov_tl(cpu_tmp0, cpu_cc_src);
> +            t1 = cpu_tmp0;
> +        }
> +
> +        tcg_gen_add_tl(reg, cpu_cc_dst, cpu_cc_src);
> +        tcg_gen_addi_tl(reg, reg, 1);
> +        gen_extu(size, reg);
> +        t0 = reg;
> +        goto adc_sbb;
> +
> +    case CC_OP_ADCB:
> +    case CC_OP_ADCW:
> +    case CC_OP_ADCL:
> +    case CC_OP_ADCQ:
> +        /* (DATA_TYPE)CC_DST <= (DATA_TYPE)CC_SRC */
> +        size = (s->cc_op - CC_OP_ADDB) & 3;
> +        t1 = gen_ext_tl(cpu_tmp0, cpu_cc_src, size, false);
> +        t0 = gen_ext_tl(reg, cpu_cc_dst, size, false);
> +    adc_sbb:
> +        tcg_gen_setcond_tl(inv ? TCG_COND_GTU : TCG_COND_LEU, reg, t0, t1);
> +        return;

There's no point in handling these, because you can never see them
assigned to s->cc_op.  The ADC/SBB translators always set CC_OP_DYNAMIC
after dynamically selecting CC_OP_ADD or CC_OP_ADC based on the carry-in.

> +    default:
> +        abort();

Better to just treat unlisted codes as dynamic?  I.e.

    default: /* Including CC_OP_DYNAMIC */
        gen_compute_eflags(s);
        /* FALLTHRU */

    case CC_OP_EFLAGS:
        ...

All that said, the patch as written is correct.

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


r~

Il 09/10/2012 22:07, Richard Henderson ha scritto:
>> > +    case CC_OP_ADCB:
>> > +    case CC_OP_ADCW:
>> > +    case CC_OP_ADCL:
>> > +    case CC_OP_ADCQ:
>> > +        /* (DATA_TYPE)CC_DST <= (DATA_TYPE)CC_SRC */
>> > +        size = (s->cc_op - CC_OP_ADDB) & 3;
>> > +        t1 = gen_ext_tl(cpu_tmp0, cpu_cc_src, size, false);
>> > +        t0 = gen_ext_tl(reg, cpu_cc_dst, size, false);
>> > +    adc_sbb:
>> > +        tcg_gen_setcond_tl(inv ? TCG_COND_GTU : TCG_COND_LEU, reg, t0, t1);
>> > +        return;
> There's no point in handling these, because you can never see them
> assigned to s->cc_op.  The ADC/SBB translators always set CC_OP_DYNAMIC
> after dynamically selecting CC_OP_ADD or CC_OP_ADC based on the carry-in.
> 

That's correct, but compared to

    case CC_OP_ADCB:
    case CC_OP_ADCW:
    case CC_OP_ADCL:
    case CC_OP_ADCQ:
    case CC_OP_SBBB:
    case CC_OP_SBBW:
    case CC_OP_SBBL:
    case CC_OP_SBBQ:
        /* There's no point in handling these, because you can never
         * see them assigned to s->cc_op.  The ADC/SBB translators
         * always set CC_OP_DYNAMIC after dynamically selecting
         * CC_OP_ADD or CC_OP_ADC based on the carry-in.
         */
         abort();

it's not a great saving and it's a bit less self-documenting...

Paolo