diff mbox

[v2,1/3] target-m68k: add cmpm

Message ID 1477604609-2206-2-git-send-email-laurent@vivier.eu (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Vivier Oct. 27, 2016, 9:43 p.m. UTC 
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 target-m68k/translate.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Richard Henderson Nov. 1, 2016, 5:48 p.m. UTC | #1 
On 10/27/2016 03:43 PM, Laurent Vivier wrote:
> +DISAS_INSN(cmpm)
> +{
> +    int opsize = insn_opsize(insn);
> +    TCGv tmp = tcg_temp_new();
> +    TCGv src, dst, addr;
> +
> +    src = gen_load(s, opsize, AREG(insn, 0), 1);
> +    /* delay the update after the second gen_load() */
> +    tcg_gen_addi_i32(tmp, AREG(insn, 0), opsize_bytes(opsize));
> +
> +    /* but if the we use the same address register to
> +     * read the second value, we must use the updated address
> +     */
> +    if (REG(insn, 0) == REG(insn, 9)) {
> +        addr = tmp;
> +    } else {
> +        addr = AREG(insn, 9);
> +    }
> +
> +    dst = gen_load(s, opsize, addr, 1);
> +    tcg_gen_mov_i32(AREG(insn, 0), tmp);
> +    tcg_gen_addi_i32(AREG(insn, 9), addr, opsize_bytes(opsize));

I wonder if we should make this more generic.
I'm thinking of transparently fixing

     case 3: /* Indirect postincrement.  */
         reg = AREG(insn, 0);
         result = gen_ldst(s, opsize, reg, val, what);
         /* ??? This is not exception safe.  The instruction may still
            fault after this point.  */
         if (what == EA_STORE || !addrp)
             tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
         return result;

If we add to DisasContext

	unsigned writeback_mask;
	TCGv writeback[8];

Then

static TCGv get_areg(DisasContext *s, unsigned regno)
{
     if (s->writeback_mask & (1 << regno)) {
         return s->writeback[regno];
     } else {
         return cpu_aregs[regno];
     }
}

static void delay_set_areg(DisasContext *s, unsigned regno,
                            TCGv val, bool give_temp)
{
     if (s->writeback_mask & (1 << regno)) {
         tcg_gen_mov_i32(s->writeback[regno], val);
         if (give_temp) {
             tcg_temp_free(val);
         }
     } else {
         s->writeback_mask |= 1 << regno;
         if (give_temp) {
             s->writeback[regno] = val;
         } else {
             TCGv tmp = tcg_temp_new();
             tcg_gen_mov_i32(tmp, val);
             s->writeback[regno] = tmp;
         }
     }
}

static void do_writebacks(DisasContext *s)
{
     unsigned mask = s->writeback_mask;
     if (mask) {
         s->writeback_mask = 0;
         do {
             unsigned regno = ctz32(mask);
             tcg_gen_mov_i32(cpu_aregs[regno], s->writeback[regno]);
             tcg_temp_free(s->writeback[regno]);
             mask &= mask - 1;
         } while (mask);
     }
}

where get_areg is used within the AREG macro, delay_set_areg is used in those 
cases where pre- and post-increment are needed, and do_writebacks is invoked at 
the end of disas_m68k_insn.

This all pre-supposes that cmpm is not a special-case within the ISA, that any 
time one reuses a register after modification one sees the new value.  Given 
the existing code within gen_ea, this would seem to be the case.

Thoughts?


r~
Laurent Vivier Nov. 1, 2016, 7:58 p.m. UTC | #2 
Le 01/11/2016 à 18:48, Richard Henderson a écrit :
> On 10/27/2016 03:43 PM, Laurent Vivier wrote:
>> +DISAS_INSN(cmpm)
>> +{
>> +    int opsize = insn_opsize(insn);
>> +    TCGv tmp = tcg_temp_new();
>> +    TCGv src, dst, addr;
>> +
>> +    src = gen_load(s, opsize, AREG(insn, 0), 1);
>> +    /* delay the update after the second gen_load() */
>> +    tcg_gen_addi_i32(tmp, AREG(insn, 0), opsize_bytes(opsize));
>> +
>> +    /* but if the we use the same address register to
>> +     * read the second value, we must use the updated address
>> +     */
>> +    if (REG(insn, 0) == REG(insn, 9)) {
>> +        addr = tmp;
>> +    } else {
>> +        addr = AREG(insn, 9);
>> +    }
>> +
>> +    dst = gen_load(s, opsize, addr, 1);
>> +    tcg_gen_mov_i32(AREG(insn, 0), tmp);
>> +    tcg_gen_addi_i32(AREG(insn, 9), addr, opsize_bytes(opsize));
> 
> I wonder if we should make this more generic.
> I'm thinking of transparently fixing
> 
>     case 3: /* Indirect postincrement.  */
>         reg = AREG(insn, 0);
>         result = gen_ldst(s, opsize, reg, val, what);
>         /* ??? This is not exception safe.  The instruction may still
>            fault after this point.  */
>         if (what == EA_STORE || !addrp)
>             tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
>         return result;
> 
> If we add to DisasContext
> 
>     unsigned writeback_mask;
>     TCGv writeback[8];
> 
> Then
> 
> static TCGv get_areg(DisasContext *s, unsigned regno)
> {
>     if (s->writeback_mask & (1 << regno)) {
>         return s->writeback[regno];
>     } else {
>         return cpu_aregs[regno];
>     }
> }
> 
> static void delay_set_areg(DisasContext *s, unsigned regno,
>                            TCGv val, bool give_temp)
> {
>     if (s->writeback_mask & (1 << regno)) {
>         tcg_gen_mov_i32(s->writeback[regno], val);
>         if (give_temp) {
>             tcg_temp_free(val);
>         }
>     } else {
>         s->writeback_mask |= 1 << regno;
>         if (give_temp) {
>             s->writeback[regno] = val;
>         } else {
>             TCGv tmp = tcg_temp_new();
>             tcg_gen_mov_i32(tmp, val);
>             s->writeback[regno] = tmp;
>         }
>     }
> }
> 
> static void do_writebacks(DisasContext *s)
> {
>     unsigned mask = s->writeback_mask;
>     if (mask) {
>         s->writeback_mask = 0;
>         do {
>             unsigned regno = ctz32(mask);
>             tcg_gen_mov_i32(cpu_aregs[regno], s->writeback[regno]);
>             tcg_temp_free(s->writeback[regno]);
>             mask &= mask - 1;
>         } while (mask);
>     }
> }
> 
> where get_areg is used within the AREG macro, delay_set_areg is used in
> those cases where pre- and post-increment are needed, and do_writebacks
> is invoked at the end of disas_m68k_insn.
> 
> This all pre-supposes that cmpm is not a special-case within the ISA,
> that any time one reuses a register after modification one sees the new
> value.  Given the existing code within gen_ea, this would seem to be the
> case.
> 
> Thoughts?

I think it's really a good idea to manage this in a generic way.
As you have already written 90% of the code, do you want to provide a
patch? I can test this with coldfire kernel and 68020 linux-user container.

Thanks,
Laurent
diff mbox

Patch

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index ee0ffe3..92e67eb 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2173,6 +2173,33 @@  DISAS_INSN(cmpa)
     gen_update_cc_cmp(s, reg, src, opsize);
 }
 
+DISAS_INSN(cmpm)
+{
+    int opsize = insn_opsize(insn);
+    TCGv tmp = tcg_temp_new();
+    TCGv src, dst, addr;
+
+    src = gen_load(s, opsize, AREG(insn, 0), 1);
+    /* delay the update after the second gen_load() */
+    tcg_gen_addi_i32(tmp, AREG(insn, 0), opsize_bytes(opsize));
+
+    /* but if the we use the same address register to
+     * read the second value, we must use the updated address
+     */
+    if (REG(insn, 0) == REG(insn, 9)) {
+        addr = tmp;
+    } else {
+        addr = AREG(insn, 9);
+    }
+
+    dst = gen_load(s, opsize, addr, 1);
+    tcg_gen_mov_i32(AREG(insn, 0), tmp);
+    tcg_gen_addi_i32(AREG(insn, 9), addr, opsize_bytes(opsize));
+    tcg_temp_free(tmp);
+
+    gen_update_cc_cmp(s, dst, src, opsize);
+}
+
 DISAS_INSN(eor)
 {
     TCGv src;
@@ -3414,6 +3441,7 @@  void register_m68k_insns (CPUM68KState *env)
     INSN(cmpa,      b1c0, f1c0, CF_ISA_A);
     INSN(cmp,       b000, f100, M68000);
     INSN(eor,       b100, f100, M68000);
+    INSN(cmpm,      b108, f138, M68000);
     INSN(cmpa,      b0c0, f0c0, M68000);
     INSN(eor,       b180, f1c0, CF_ISA_A);
     BASE(and,       c000, f000);