Re: [Qemu-devel] [PATCH for-2.5 17/30] m68k: ori/andi/subi/addi/eori/cmpi can modify SR/CCR

[Richard Henderson <rth@twiddle.net>]

On 08/09/2015 01:13 PM, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  target-m68k/translate.c | 95 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 58 insertions(+), 37 deletions(-)
> 
> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index 6a426e1..9e379b3 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -1343,6 +1343,42 @@ DISAS_INSN(bitop_im)
>          DEST_EA(env, insn, opsize, tmp, &addr);
>      }
>  }
> +
> +static TCGv gen_get_ccr(DisasContext *s)
> +{
> +    TCGv dest;
> +
> +    gen_flush_flags(s);
> +    dest = tcg_temp_new();
> +    tcg_gen_shli_i32(dest, QREG_CC_X, 4);
> +    tcg_gen_or_i32(dest, dest, QREG_CC_DEST);
> +    return dest;
> +}
> +
> +static TCGv gen_get_sr(DisasContext *s)
> +{
> +    TCGv ccr;
> +    TCGv sr;
> +
> +    ccr = gen_get_ccr(s);
> +    sr = tcg_temp_new();
> +    tcg_gen_andi_i32(sr, QREG_SR, 0xffe0);
> +    tcg_gen_or_i32(sr, sr, ccr);
> +    return sr;
> +}

Leaking the temporary produced by gen_get_ccr.

> +
> +static void gen_set_sr(DisasContext *s, TCGv val, int ccr_only)
> +{
> +    TCGv tmp;
> +    tmp = tcg_temp_new();
> +    tcg_gen_andi_i32(QREG_CC_DEST, val, 0xf);
> +    tcg_gen_shri_i32(tmp, val, 4);
> +    tcg_gen_andi_i32(QREG_CC_X, tmp, 1);
> +    if (!ccr_only) {
> +        gen_helper_set_sr(cpu_env, val);
> +    }
> +}

Leaking tmp.  And you don't even need to allocate it -- perform the shift into
QREG_CC_X.

> +
>  DISAS_INSN(arith_im)
>  {
>      int op;
> @@ -1367,7 +1403,20 @@ DISAS_INSN(arith_im)
>      default:
>         abort();
>      }
> -    SRC_EA(env, src1, opsize, -1, (op == 6) ? NULL : &addr);
> +    if ((op == 0 || op == 1) &&

The subject line is surely misleading, as this is only ANDI/ORI, right?  Again,
some more commentary would be helpful.

> +        (insn & 0x3f) == 0x3c) {
> +        if (opsize == OS_BYTE) {
> +            src1 = gen_get_ccr(s);
> +        } else {
> +            if (IS_USER(s)) {
> +                gen_exception(s, s->pc - 2, EXCP_PRIVILEGE);
> +                return;
> +            }
> +            src1 = gen_get_sr(s);
> +        }
> +    } else {
> +        SRC_EA(env, src1, opsize, -1, (op == 6) ? NULL : &addr);
> +    }
>      dest = tcg_temp_new();
>      switch (op) {
>      case 0: /* ori */
> @@ -1405,7 +1454,14 @@ DISAS_INSN(arith_im)
>      default:
>          abort();
>      }
> -    DEST_EA(env, insn, opsize, dest, &addr);
> +    if (op != 6) {
> +        if ((op == 0 || op == 1) &&
> +            (insn & 0x3f) == 0x3c) {
> +            gen_set_sr(s, dest, opsize == OS_BYTE);
> +        } else {
> +            DEST_EA(env, insn, opsize, dest, &addr);
> +        }
> +    }

That said, I think this code should be rearranged so that you don't have to
replicate so many conditionals.  In particular, the only thing of use in the
middle of import for the ccr insns are two lines: tcg_gen_andi/ori_tl.

I think it would be better to structure as

  if ((insn & 0x3f) == 0x3c && (op == 0 || op == 1)) {
    if (opsize == OS_BYTE) {
      src1 = gen_get_ccr ();
    } else {
      ...
    }
    if (op == 0) {
      tcg_gen_ori_i32 ...
    } else {
      tcg_gen_andi_i32 ...
    }
    gen_set_sr(s, dest, opsize == OS_BYTE);
    return;
  }

  // existing code


r~