Re: [Qemu-devel] [PATCH 35/52] target-m68k: inline rotate ops

[Richard Henderson]

On 05/04/2016 11:08 AM, Laurent Vivier wrote:
> +static inline void rotate_x_flags(TCGv reg, int size)
> +{
> +    switch (size) {
> +    case 8:
> +        tcg_gen_ext8s_i32(reg, reg);
> +        break;
> +    case 16:
> +        tcg_gen_ext16s_i32(reg, reg);
> +        break;
> +    default:
> +        break;
> +    }
> +    tcg_gen_mov_i32(QREG_CC_N, reg);
> +    tcg_gen_mov_i32(QREG_CC_Z, reg);
> +    tcg_gen_mov_i32(QREG_CC_C, QREG_CC_X);

A comment saying that CC_V has already been set wouldn't go amiss.

Alternately, don't work so hard to re-use CC_V as a zero in the rotate_reg case.

> +            tcg_gen_shli_i32(X, reg, 1);
> +            tcg_gen_or_i32(X, X, QREG_CC_X);

This can clobber CC_X before it's used.

I think you need to be less clever about passing in QREG_CC_X as X in 
rotate*_im and instead always return a temporary.

> +static inline void rotate_x(TCGv dest, TCGv X, TCGv reg, TCGv shift,
> +                            int left, int size)
> +{
> +    TCGv_i64 t0, shift64;

I can't help but think it wouldn't be better to only use 64-bit shifts when 
size == 32.  You can implement the 8- and 16-bit rotates with a 32-bit ro

> +DISAS_INSN(rotate_reg)
> +{
> +    TCGv reg;
> +    TCGv src;
> +    TCGv tmp, t0;
> +    int left = (insn & 0x100);
> +
> +    reg = DREG(insn, 0);
> +    src = DREG(insn, 9);
> +    tmp = tcg_temp_new_i32();
> +    if (insn & 8) {
> +        tcg_gen_andi_i32(tmp, src, 31);
> +        rotate(reg, tmp, left, 32);
> +        /* if shift == 0, clear C */
> +        tcg_gen_andi_i32(tmp, src, 63);

If reg == src, you'll compute the wrong results here.

> +    } else {
> +        TCGv dest, X;
> +        dest = tcg_temp_new();
> +        X = tcg_temp_new();
> +        /* shift in [0..63] */
> +        tcg_gen_andi_i32(tmp, src, 63);
> +        /* modulo 33 */
> +        t0 = tcg_const_i32(33);
> +        tcg_gen_remu_i32(tmp, tmp, t0);
> +        tcg_temp_free(t0);
> +        rotate_x(dest, X, reg, tmp, left, 32);
> +        tcg_gen_movcond_i32(TCG_COND_EQ, QREG_CC_X,
> +                            tmp, QREG_CC_V /* 0 */,
> +                            QREG_CC_X /* 0 */, X);

And here you can't use the mod 33 shift count, but the full mod 64 shift count.

Similarly for the 8 and 16 bit versions.


r~