diff mbox

[v2] target-m68k: add rol/ror/roxl/roxr instructions

Message ID 1478818284-9482-1-git-send-email-laurent@vivier.eu (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Vivier Nov. 10, 2016, 10:51 p.m. UTC 
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2:
- use shift to do rotate_x() for 8 and 16bit value
- rotate_x()/rotate32_x() are  a no-op when shift % (size + 1) == 0
- add some missing tcg_temp_free()

 target-m68k/translate.c | 414 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 414 insertions(+)

Comments

Richard Henderson Nov. 11, 2016, 7:31 a.m. UTC | #1 
On 11/10/2016 11:51 PM, Laurent Vivier wrote:
> +/* Result of rotate_x() is valid if 0 < shift < (size + 1) < 32 */
> +static TCGv rotate_x(TCGv dest, TCGv src, TCGv shift, int left, int size)
> +{
> +    TCGv X, shl, shr, shx;
> +
> +    shr = tcg_temp_new();
> +    shl = tcg_temp_new();
> +    shx = tcg_temp_new();
> +    if (left) {
> +        tcg_gen_mov_i32(shl, shift);      /* shl = shift */
> +        tcg_gen_movi_i32(shr, size + 1);
> +        tcg_gen_sub_i32(shr, shr, shift); /* shr = size + 1 - shift */
> +        tcg_gen_subi_i32(shx, shift, 1);  /* shx = shift - 1 */

You're failing to bound shx properly.  If shift == 0, you produce -1 here which 
is illegal.  You could mask this to 31, but it's even better to produce size, 
as I said before, because then you don't have to have

> +        tcg_gen_movcond_i32(TCG_COND_EQ, X,
> +                            t1, zero,
> +                            QREG_CC_X, X);
> +        tcg_gen_movcond_i32(TCG_COND_EQ, DREG(insn, 0),
> +                            t1, zero,
> +                            DREG(insn, 0), res);

this at the end.  A zero shift will simply leave everything where it belongs, 
and you extract all the parts as normal.

> +DISAS_INSN(rotate_mem)
> +{
> +    TCGv src;
> +    TCGv addr;
> +    TCGv shift;
> +    int left = (insn & 0x100);
> +
> +    SRC_EA(env, src, OS_WORD, 0, &addr);
> +
> +    shift = tcg_const_i32(1);
> +    if (insn & 8) {

It's bit 9 for memory rotate.


r~
Laurent Vivier Nov. 11, 2016, 11:21 a.m. UTC | #2 
Le 11/11/2016 à 08:31, Richard Henderson a écrit :
> On 11/10/2016 11:51 PM, Laurent Vivier wrote:
>> +/* Result of rotate_x() is valid if 0 < shift < (size + 1) < 32 */
>> +static TCGv rotate_x(TCGv dest, TCGv src, TCGv shift, int left, int
>> size)
>> +{
>> +    TCGv X, shl, shr, shx;
>> +
>> +    shr = tcg_temp_new();
>> +    shl = tcg_temp_new();
>> +    shx = tcg_temp_new();
>> +    if (left) {
>> +        tcg_gen_mov_i32(shl, shift);      /* shl = shift */
>> +        tcg_gen_movi_i32(shr, size + 1);
>> +        tcg_gen_sub_i32(shr, shr, shift); /* shr = size + 1 - shift */
>> +        tcg_gen_subi_i32(shx, shift, 1);  /* shx = shift - 1 */
> 
> You're failing to bound shx properly.  If shift == 0, you produce -1
> here which is illegal.  You could mask this to 31, but it's even better

Yes, but in this case the result is ignored by the following movcond.

tcg/README says:

* shl_i32/i64 t0, t1, t2

t0=t1 << t2. Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)

* shr_i32/i64 t0, t1, t2

t0=t1 >> t2 (unsigned). Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)

An operation with "unspecified behavior" shall not crash.  However,
the result may be one of several possibilities so may be considered
an "undefined result".

> to produce size, as I said before, because then you don't have to have

Perhaps I've missed something (again), but I've tested with
"int shr = shl ^ (size - 1);" and it produces the wrong result.

for instance size = 8 and shift = 1

shr = 1 ^ 7 = 6

should be: 8 + 1 - 1 = 8

> 
>> +        tcg_gen_movcond_i32(TCG_COND_EQ, X,
>> +                            t1, zero,
>> +                            QREG_CC_X, X);
>> +        tcg_gen_movcond_i32(TCG_COND_EQ, DREG(insn, 0),
>> +                            t1, zero,
>> +                            DREG(insn, 0), res);
> 
> this at the end.  A zero shift will simply leave everything where it
> belongs, and you extract all the parts as normal.

To do "int shx = shl ? shl - 1 : 16;", we need a "movcond" in every
case, which should not be needed in the case of "rotate_im" where shift
is between 1 and 8.

> 
>> +DISAS_INSN(rotate_mem)
>> +{
>> +    TCGv src;
>> +    TCGv addr;
>> +    TCGv shift;
>> +    int left = (insn & 0x100);
>> +
>> +    SRC_EA(env, src, OS_WORD, 0, &addr);
>> +
>> +    shift = tcg_const_i32(1);
>> +    if (insn & 8) {
> 
> It's bit 9 for memory rotate.

right. I fix that.

Thanks,
Laurent
Richard Henderson Nov. 11, 2016, 2:07 p.m. UTC | #3 
On 11/11/2016 12:21 PM, Laurent Vivier wrote:
> Le 11/11/2016 à 08:31, Richard Henderson a écrit :
>> On 11/10/2016 11:51 PM, Laurent Vivier wrote:
>>> +/* Result of rotate_x() is valid if 0 < shift < (size + 1) < 32 */
>>> +static TCGv rotate_x(TCGv dest, TCGv src, TCGv shift, int left, int
>>> size)
>>> +{
>>> +    TCGv X, shl, shr, shx;
>>> +
>>> +    shr = tcg_temp_new();
>>> +    shl = tcg_temp_new();
>>> +    shx = tcg_temp_new();
>>> +    if (left) {
>>> +        tcg_gen_mov_i32(shl, shift);      /* shl = shift */
>>> +        tcg_gen_movi_i32(shr, size + 1);
>>> +        tcg_gen_sub_i32(shr, shr, shift); /* shr = size + 1 - shift */
>>> +        tcg_gen_subi_i32(shx, shift, 1);  /* shx = shift - 1 */
>>
>> You're failing to bound shx properly.  If shift == 0, you produce -1
>> here which is illegal.  You could mask this to 31, but it's even better
>
> Yes, but in this case the result is ignored by the following movcond.
>
> tcg/README says:
>
> * shl_i32/i64 t0, t1, t2
>
> t0=t1 << t2. Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)
>
> * shr_i32/i64 t0, t1, t2
>
> t0=t1 >> t2 (unsigned). Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64)
>
> An operation with "unspecified behavior" shall not crash.  However,
> the result may be one of several possibilities so may be considered
> an "undefined result".
>
>> to produce size, as I said before, because then you don't have to have
>
> Perhaps I've missed something (again), but I've tested with
> "int shr = shl ^ (size - 1);" and it produces the wrong result.

Huh?  I'm talking about

   shx = shl - 1;
   shx = shx < 0 ? size : shx;

> To do "int shx = shl ? shl - 1 : 16;", we need a "movcond" in every
> case, which should not be needed in the case of "rotate_im" where shift
> is between 1 and 8.

For constant input, the movcond will be folded to a constant.  Along with all 
of the other shr/shl/shx computation.


r~
diff mbox

Patch

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 96af672..0a85dc4 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3080,6 +3080,413 @@  DISAS_INSN(shift_mem)
     set_cc_op(s, CC_OP_FLAGS);
 }
 
+static void rotate(TCGv reg, TCGv shift, int left, int size)
+{
+    switch (size) {
+    case 8:
+        /* Replicate the 8-bit input so that a 32-bit rotate works.  */
+        tcg_gen_ext8u_i32(reg, reg);
+        tcg_gen_muli_i32(reg, reg, 0x01010101);
+        goto do_long;
+    case 16:
+        /* Replicate the 16-bit input so that a 32-bit rotate works.  */
+        tcg_gen_deposit_i32(reg, reg, reg, 16, 16);
+        goto do_long;
+    do_long:
+    default:
+        if (left) {
+            tcg_gen_rotl_i32(reg, reg, shift);
+        } else {
+            tcg_gen_rotr_i32(reg, reg, shift);
+        }
+    }
+
+    /* compute flags */
+
+    switch (size) {
+    case 8:
+        tcg_gen_ext8s_i32(reg, reg);
+        break;
+    case 16:
+        tcg_gen_ext16s_i32(reg, reg);
+        break;
+    default:
+        break;
+    }
+
+    /* QREG_CC_X is not affected */
+
+    tcg_gen_mov_i32(QREG_CC_N, reg);
+    tcg_gen_mov_i32(QREG_CC_Z, reg);
+
+    if (left) {
+        tcg_gen_andi_i32(QREG_CC_C, reg, 1);
+    } else {
+        tcg_gen_shri_i32(QREG_CC_C, reg, 31);
+    }
+
+    tcg_gen_movi_i32(QREG_CC_V, 0); /* always cleared */
+}
+
+static void rotate_x_flags(TCGv reg, TCGv X, 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_X, X);
+    tcg_gen_mov_i32(QREG_CC_C, X);
+    tcg_gen_movi_i32(QREG_CC_V, 0);
+}
+
+/* Result of rotate_x() is valid if 0 < shift < (size + 1) < 32 */
+static TCGv rotate_x(TCGv dest, TCGv src, TCGv shift, int left, int size)
+{
+    TCGv X, shl, shr, shx;
+
+    shr = tcg_temp_new();
+    shl = tcg_temp_new();
+    shx = tcg_temp_new();
+    if (left) {
+        tcg_gen_mov_i32(shl, shift);      /* shl = shift */
+        tcg_gen_movi_i32(shr, size + 1);
+        tcg_gen_sub_i32(shr, shr, shift); /* shr = size + 1 - shift */
+        tcg_gen_subi_i32(shx, shift, 1);  /* shx = shift - 1 */
+    } else {
+        tcg_gen_mov_i32(shr, shift);      /* shr = shift */
+        tcg_gen_movi_i32(shl, size + 1);
+        tcg_gen_sub_i32(shl, shl, shift); /* shl = size + 1 - shift */
+        tcg_gen_movi_i32(shx, size);
+        tcg_gen_sub_i32(shx, shx, shift); /* shx = size - shift */
+    }
+
+    /* dest = (src << shl) | (src >> shr) | (x << shx); */
+
+    tcg_gen_shl_i32(shl, src, shl);
+    tcg_gen_shr_i32(shr, src, shr);
+    tcg_gen_or_i32(dest, shl, shr);
+    tcg_temp_free(shl);
+    tcg_temp_free(shr);
+    tcg_gen_shl_i32(shx, QREG_CC_X, shx);
+    tcg_gen_or_i32(dest, dest, shx);
+    tcg_temp_free(shx);
+
+    /* X = (dest >> size) & 1 */
+
+    X = tcg_temp_new();
+    tcg_gen_shri_i32(X, dest, size);
+    tcg_gen_andi_i32(X, X, 1);
+
+    return X;
+}
+
+/* Result of rotate32_x() is valid if 0 < shift < 33 */
+static TCGv rotate32_x(TCGv dest, TCGv src, TCGv shift, int left)
+{
+    TCGv_i64 t0, shift64;
+    TCGv X, lo, hi;
+
+    shift64 = tcg_temp_new_i64();
+    tcg_gen_extu_i32_i64(shift64, shift);
+
+    t0 = tcg_temp_new_i64();
+
+    X = tcg_temp_new();
+    lo = tcg_temp_new();
+    hi = tcg_temp_new();
+
+    if (left) {
+        /* create [src:X:..] */
+
+        tcg_gen_shli_i32(lo, QREG_CC_X, 31);
+        tcg_gen_concat_i32_i64(t0, lo, src);
+
+        /* rotate */
+
+        tcg_gen_rotl_i64(t0, t0, shift64);
+        tcg_temp_free_i64(shift64);
+
+        /* result is [src:..:src:X] */
+
+        tcg_gen_extr_i64_i32(lo, hi, t0);
+        tcg_gen_andi_i32(X, lo, 1);
+
+        tcg_gen_shri_i32(lo, lo, 1);
+    } else {
+        /* create [..:X:src] */
+
+        tcg_gen_concat_i32_i64(t0, src, QREG_CC_X);
+
+        tcg_gen_rotr_i64(t0, t0, shift64);
+        tcg_temp_free_i64(shift64);
+
+        /* result is value: [X:src:..:src] */
+
+        tcg_gen_extr_i64_i32(lo, hi, t0);
+
+        /* extract X */
+
+        tcg_gen_shri_i32(X, hi, 31);
+
+        /* extract result */
+
+        tcg_gen_shli_i32(hi, hi, 1);
+    }
+    tcg_gen_or_i32(dest, lo, hi);
+    tcg_temp_free(hi);
+    tcg_temp_free(lo);
+    tcg_temp_free_i64(t0);
+
+    return X;
+}
+
+DISAS_INSN(rotate_im)
+{
+    TCGv shift;
+    int tmp;
+    int left = (insn & 0x100);
+
+    tmp = (insn >> 9) & 7;
+    if (tmp == 0) {
+        tmp = 8;
+    }
+
+    shift = tcg_const_i32(tmp);
+    if (insn & 8) {
+        rotate(DREG(insn, 0), shift, left, 32);
+    } else {
+        TCGv X = rotate32_x(DREG(insn, 0), DREG(insn, 0), shift, left);
+        rotate_x_flags(DREG(insn, 0), X, 32);
+        tcg_temp_free(X);
+    }
+    tcg_temp_free(shift);
+
+    set_cc_op(s, CC_OP_FLAGS);
+}
+
+DISAS_INSN(rotate8_im)
+{
+    int left = (insn & 0x100);
+    TCGv reg;
+    TCGv shift;
+    int tmp;
+
+    reg = gen_extend(DREG(insn, 0), OS_BYTE, 0);
+
+    tmp = (insn >> 9) & 7;
+    if (tmp == 0) {
+        tmp = 8;
+    }
+
+    shift = tcg_const_i32(tmp);
+    if (insn & 8) {
+        rotate(reg, shift, left, 8);
+    } else {
+        TCGv X = rotate_x(reg, reg, shift, left, 8);
+        rotate_x_flags(reg, X, 8);
+        tcg_temp_free(X);
+    }
+    tcg_temp_free(shift);
+    gen_partset_reg(OS_BYTE, DREG(insn, 0), reg);
+    set_cc_op(s, CC_OP_FLAGS);
+}
+
+DISAS_INSN(rotate16_im)
+{
+    int left = (insn & 0x100);
+    TCGv reg;
+    TCGv shift;
+    int tmp;
+
+    reg = gen_extend(DREG(insn, 0), OS_WORD, 0);
+    tmp = (insn >> 9) & 7;
+    if (tmp == 0) {
+        tmp = 8;
+    }
+
+    shift = tcg_const_i32(tmp);
+    if (insn & 8) {
+        rotate(reg, shift, left, 16);
+    } else {
+        TCGv X = rotate_x(reg, reg, shift, left, 16);
+        rotate_x_flags(reg, X, 8);
+        tcg_temp_free(X);
+    }
+    tcg_temp_free(shift);
+    gen_partset_reg(OS_WORD, DREG(insn, 0), reg);
+    set_cc_op(s, CC_OP_FLAGS);
+}
+
+DISAS_INSN(rotate_reg)
+{
+    TCGv reg;
+    TCGv src;
+    TCGv t0, t1;
+    int left = (insn & 0x100);
+
+    reg = DREG(insn, 0);
+    src = DREG(insn, 9);
+    t0 = tcg_temp_new();
+    t1 = tcg_temp_new_i32();
+    if (insn & 8) {
+        tcg_gen_andi_i32(t0, src, 63);
+        tcg_gen_andi_i32(t1, src, 31);
+        rotate(reg, t1, left, 32);
+        /* if shift == 0, clear C */
+        tcg_gen_movcond_i32(TCG_COND_EQ, QREG_CC_C,
+                            t0, QREG_CC_V /* 0 */,
+                            QREG_CC_V /* 0 */, QREG_CC_C);
+    } else {
+        TCGv X, zero, res;
+        /* shift in [0..63] */
+        tcg_gen_andi_i32(t0, src, 63);
+        /* modulo 33 */
+        tcg_gen_movi_i32(t1, 33);
+        tcg_gen_remu_i32(t1, t0, t1);
+        res = tcg_temp_new();
+        X = rotate32_x(res, DREG(insn, 0), t1, left);
+        /* if shift % 33 == 0, register and X are not affected */
+        zero = tcg_const_i32(0);
+        tcg_gen_movcond_i32(TCG_COND_EQ, X,
+                            t1, zero,
+                            QREG_CC_X, X);
+        tcg_gen_movcond_i32(TCG_COND_EQ, DREG(insn, 0),
+                            t1, zero,
+                            DREG(insn, 0), res);
+        tcg_temp_free(res);
+        tcg_temp_free(zero);
+        rotate_x_flags(DREG(insn, 0), X, 32);
+        tcg_temp_free(X);
+    }
+    tcg_temp_free(t1);
+    tcg_temp_free(t0);
+    set_cc_op(s, CC_OP_FLAGS);
+}
+
+DISAS_INSN(rotate8_reg)
+{
+    TCGv reg;
+    TCGv src;
+    TCGv t0, t1;
+    int left = (insn & 0x100);
+
+    reg = gen_extend(DREG(insn, 0), OS_BYTE, 0);
+    src = DREG(insn, 9);
+    t0 = tcg_temp_new_i32();
+    t1 = tcg_temp_new_i32();
+    if (insn & 8) {
+        tcg_gen_andi_i32(t0, src, 63);
+        tcg_gen_andi_i32(t1, src, 7);
+        rotate(reg, t1, left, 8);
+        /* if shift == 0, clear C */
+        tcg_gen_movcond_i32(TCG_COND_EQ, QREG_CC_C,
+                            t0, QREG_CC_V /* 0 */,
+                            QREG_CC_V /* 0 */, QREG_CC_C);
+    } else {
+        TCGv X, res, zero;
+        /* shift in [0..63] */
+        tcg_gen_andi_i32(t0, src, 63);
+        /* modulo 9 */
+        tcg_gen_movi_i32(t1, 9);
+        tcg_gen_remu_i32(t1, t0, t1);
+        res = tcg_temp_new();
+        X = rotate_x(res, reg, t1, left, 8);
+        /* if shift % 9 == 0, register and X are not affected */
+        zero = tcg_const_i32(0);
+        tcg_gen_movcond_i32(TCG_COND_EQ, X,
+                            t1, zero,
+                            QREG_CC_X, X);
+        tcg_gen_movcond_i32(TCG_COND_EQ, reg,
+                            t1, zero,
+                            reg, res);
+        tcg_temp_free(res);
+        tcg_temp_free(zero);
+        rotate_x_flags(reg, X, 8);
+        tcg_temp_free(X);
+    }
+    tcg_temp_free(t1);
+    tcg_temp_free(t0);
+    gen_partset_reg(OS_BYTE, DREG(insn, 0), reg);
+    set_cc_op(s, CC_OP_FLAGS);
+}
+
+DISAS_INSN(rotate16_reg)
+{
+    TCGv reg;
+    TCGv src;
+    TCGv t0, t1;
+    int left = (insn & 0x100);
+
+    reg = gen_extend(DREG(insn, 0), OS_WORD, 0);
+    src = DREG(insn, 9);
+    t0 = tcg_temp_new_i32();
+    t1 = tcg_temp_new_i32();
+    if (insn & 8) {
+        tcg_gen_andi_i32(t0, src, 63);
+        tcg_gen_andi_i32(t1, src, 15);
+        rotate(reg, t1, left, 16);
+        /* if shift == 0, clear C */
+        tcg_gen_movcond_i32(TCG_COND_EQ, QREG_CC_C,
+                            t0, QREG_CC_V /* 0 */,
+                            QREG_CC_V /* 0 */, QREG_CC_C);
+    } else {
+        TCGv X, res, zero;
+        /* shift in [0..63] */
+        tcg_gen_andi_i32(t0, src, 63);
+        /* modulo 17 */
+        t1 = tcg_const_i32(17);
+        tcg_gen_remu_i32(t1, t0, t1);
+        res = tcg_temp_new();
+        X = rotate_x(res, reg, t1, left, 16);
+        /* if shift % 17 == 0, register and X are not affected */
+        zero = tcg_const_i32(0);
+        tcg_gen_movcond_i32(TCG_COND_EQ, X,
+                            t1, zero,
+                            QREG_CC_X, X);
+        tcg_gen_movcond_i32(TCG_COND_EQ, reg,
+                            t1, zero,
+                            reg, res);
+        tcg_temp_free(res);
+        tcg_temp_free(zero);
+        rotate_x_flags(reg, X, 16);
+        tcg_temp_free(X);
+    }
+    tcg_temp_free(t1);
+    tcg_temp_free(t0);
+    gen_partset_reg(OS_WORD, DREG(insn, 0), reg);
+    set_cc_op(s, CC_OP_FLAGS);
+}
+
+DISAS_INSN(rotate_mem)
+{
+    TCGv src;
+    TCGv addr;
+    TCGv shift;
+    int left = (insn & 0x100);
+
+    SRC_EA(env, src, OS_WORD, 0, &addr);
+
+    shift = tcg_const_i32(1);
+    if (insn & 8) {
+        rotate(src, shift, left, 16);
+    } else {
+        TCGv X = rotate_x(src, src, shift, left, 16);
+        rotate_x_flags(src, X, 16);
+        tcg_temp_free(X);
+    }
+    tcg_temp_free(shift);
+    DEST_EA(env, insn, OS_WORD, src, &addr);
+    set_cc_op(s, CC_OP_FLAGS);
+}
+
 static void bitfield_param(uint16_t ext, TCGv *offset, TCGv *width, TCGv *mask)
 {
     TCGv tmp;
@@ -4492,6 +4899,13 @@  void register_m68k_insns (CPUM68KState *env)
     INSN(shift16_reg, e060, f0f0, M68000);
     INSN(shift_reg, e0a0, f0f0, M68000);
     INSN(shift_mem, e0c0, fcc0, M68000);
+    INSN(rotate_im, e090, f0f0, M68000);
+    INSN(rotate8_im, e010, f0f0, M68000);
+    INSN(rotate16_im, e050, f0f0, M68000);
+    INSN(rotate_reg, e0b0, f0f0, M68000);
+    INSN(rotate8_reg, e030, f0f0, M68000);
+    INSN(rotate16_reg, e070, f0f0, M68000);
+    INSN(rotate_mem, e4c0, fcc0, M68000);
     INSN(bitfield_mem,e8c0, f8c0, BITFIELD);
     INSN(bitfield_reg,e8c0, f8f8, BITFIELD);
     INSN(undef_fpu, f000, f000, CF_ISA_A);