All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/4] target-m68k areg writeback
@ 2016-11-01 21:29 Richard Henderson
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 1/4] target-m68k: Delay autoinc writeback Richard Henderson
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Richard Henderson @ 2016-11-01 21:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: laurent

Here's the patch I almost wrote in the email, followed by
a cleanup that allows cmpm to be written "nicely".

I can test this to some extent with the coldfire kernel,
but of course coldfire can't excersise any of the tricky
edge cases that m68000 can.

I'm particularly interested in edge cases like

	mov.b	a0@+, a0@+
	movea	a0@+, a0
	movea	a0, a0@-

The first two are not really useful and likely not show up
in normal code.  The third may well do so; I think our
current code gets it wrong, but this will get it right.


r~


Laurent Vivier (1):
  target-m68k: add cmpm

Richard Henderson (3):
  target-m68k: Delay autoinc writeback
  target-m68k: Split gen_lea and gen_ea
  target-m68k: Use gen_ea_mode in cmpm

 target-m68k/translate.c | 207 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 136 insertions(+), 71 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/4] target-m68k: Delay autoinc writeback
  2016-11-01 21:29 [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Richard Henderson
@ 2016-11-01 21:29 ` Richard Henderson
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 2/4] target-m68k: Split gen_lea and gen_ea Richard Henderson
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2016-11-01 21:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: laurent

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-m68k/translate.c | 83 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 63 insertions(+), 20 deletions(-)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 9ad974f..f812c4b 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -59,12 +59,12 @@ static TCGv cpu_aregs[8];
 static TCGv_i64 cpu_fregs[8];
 static TCGv_i64 cpu_macc[4];
 
-#define REG(insn, pos) (((insn) >> (pos)) & 7)
+#define REG(insn, pos)  (((insn) >> (pos)) & 7)
 #define DREG(insn, pos) cpu_dregs[REG(insn, pos)]
-#define AREG(insn, pos) cpu_aregs[REG(insn, pos)]
+#define AREG(insn, pos) get_areg(s, REG(insn, pos))
 #define FREG(insn, pos) cpu_fregs[REG(insn, pos)]
-#define MACREG(acc) cpu_macc[acc]
-#define QREG_SP cpu_aregs[7]
+#define MACREG(acc)     cpu_macc[acc]
+#define QREG_SP         get_areg(s, 7)
 
 static TCGv NULL_QREG;
 #define IS_NULL_QREG(t) (TCGV_EQUAL(t, NULL_QREG))
@@ -141,8 +141,55 @@ typedef struct DisasContext {
     int singlestep_enabled;
     TCGv_i64 mactmp;
     int done_mac;
+    int writeback_mask;
+    TCGv writeback[8];
 } DisasContext;
 
+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)) {
+        if (give_temp) {
+            tcg_temp_free(s->writeback[regno]);
+            s->writeback[regno] = val;
+        } else {
+            tcg_gen_mov_i32(s->writeback[regno], val);
+        }
+    } else {
+        s->writeback_mask |= 1 << regno;
+        if (give_temp) {
+            s->writeback[regno] = val;
+        } else {
+            TCGv tmp = tcg_temp_new();
+            s->writeback[regno] = tmp;
+            tcg_gen_mov_i32(tmp, val);
+        }
+    }
+}
+
+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);
+    }
+}
+
 #define DISAS_JUMP_NEXT 4
 
 #if defined(CONFIG_USER_ONLY)
@@ -331,7 +378,7 @@ static inline uint32_t read_im32(CPUM68KState *env, DisasContext *s)
 }
 
 /* Calculate and address index.  */
-static TCGv gen_addr_index(uint16_t ext, TCGv tmp)
+static TCGv gen_addr_index(DisasContext *s, uint16_t ext, TCGv tmp)
 {
     TCGv add;
     int scale;
@@ -388,7 +435,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext *s, TCGv base)
         tmp = tcg_temp_new();
         if ((ext & 0x44) == 0) {
             /* pre-index */
-            add = gen_addr_index(ext, tmp);
+            add = gen_addr_index(s, ext, tmp);
         } else {
             add = NULL_QREG;
         }
@@ -417,7 +464,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext *s, TCGv base)
             /* memory indirect */
             base = gen_load(s, OS_LONG, add, 0);
             if ((ext & 0x44) == 4) {
-                add = gen_addr_index(ext, tmp);
+                add = gen_addr_index(s, ext, tmp);
                 tcg_gen_add_i32(tmp, add, base);
                 add = tmp;
             } else {
@@ -441,7 +488,7 @@ static TCGv gen_lea_indexed(CPUM68KState *env, DisasContext *s, TCGv base)
     } else {
         /* brief extension word format */
         tmp = tcg_temp_new();
-        add = gen_addr_index(ext, tmp);
+        add = gen_addr_index(s, ext, tmp);
         if (!IS_NULL_QREG(base)) {
             tcg_gen_add_i32(tmp, add, base);
             if ((int8_t)ext)
@@ -755,10 +802,11 @@ static TCGv gen_ea(CPUM68KState *env, DisasContext *s, uint16_t insn,
     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));
+        if (what == EA_STORE || !addrp) {
+            TCGv tmp = tcg_temp_new();
+            tcg_gen_addi_i32(tmp, reg, opsize_bytes(opsize));
+            delay_set_areg(s, REG(insn, 0), tmp, true);
+        }
         return result;
     case 4: /* Indirect predecrememnt.  */
         {
@@ -773,11 +821,8 @@ static TCGv gen_ea(CPUM68KState *env, DisasContext *s, uint16_t insn,
                     *addrp = tmp;
             }
             result = gen_ldst(s, opsize, tmp, val, what);
-            /* ??? This is not exception safe.  The instruction may still
-               fault after this point.  */
             if (what == EA_STORE || !addrp) {
-                reg = AREG(insn, 0);
-                tcg_gen_mov_i32(reg, tmp);
+                delay_set_areg(s, REG(insn, 0), tmp, false);
             }
         }
         return result;
@@ -3446,11 +3491,9 @@ void register_m68k_insns (CPUM68KState *env)
    write back the result to memory before setting the condition codes.  */
 static void disas_m68k_insn(CPUM68KState * env, DisasContext *s)
 {
-    uint16_t insn;
-
-    insn = read_im16(env, s);
-
+    uint16_t insn = read_im16(env, s);
     opcode_table[insn](env, s, insn);
+    do_writebacks(s);
 }
 
 /* generate intermediate code for basic block 'tb'.  */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/4] target-m68k: Split gen_lea and gen_ea
  2016-11-01 21:29 [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Richard Henderson
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 1/4] target-m68k: Delay autoinc writeback Richard Henderson
@ 2016-11-01 21:29 ` Richard Henderson
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 3/4] target-m68k: add cmpm Richard Henderson
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2016-11-01 21:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: laurent

Provide gen_lea_mode and gen_ea_mode, where the mode can be
specified manually, rather than taken from the instruction.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-m68k/translate.c | 112 +++++++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 53 deletions(-)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index f812c4b..00018b4 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -697,37 +697,37 @@ static void gen_partset_reg(int opsize, TCGv reg, TCGv val)
 
 /* Generate code for an "effective address".  Does not adjust the base
    register for autoincrement addressing modes.  */
-static TCGv gen_lea(CPUM68KState *env, DisasContext *s, uint16_t insn,
-                    int opsize)
+static TCGv gen_lea_mode(CPUM68KState *env, DisasContext *s,
+                         int mode, int reg0, int opsize)
 {
     TCGv reg;
     TCGv tmp;
     uint16_t ext;
     uint32_t offset;
 
-    switch ((insn >> 3) & 7) {
+    switch (mode) {
     case 0: /* Data register direct.  */
     case 1: /* Address register direct.  */
         return NULL_QREG;
     case 2: /* Indirect register */
     case 3: /* Indirect postincrement.  */
-        return AREG(insn, 0);
+        return get_areg(s, reg0);
     case 4: /* Indirect predecrememnt.  */
-        reg = AREG(insn, 0);
+        reg = get_areg(s, reg0);
         tmp = tcg_temp_new();
         tcg_gen_subi_i32(tmp, reg, opsize_bytes(opsize));
         return tmp;
     case 5: /* Indirect displacement.  */
-        reg = AREG(insn, 0);
+        reg = get_areg(s, reg0);
         tmp = tcg_temp_new();
         ext = read_im16(env, s);
         tcg_gen_addi_i32(tmp, reg, (int16_t)ext);
         return tmp;
     case 6: /* Indirect index + displacement.  */
-        reg = AREG(insn, 0);
+        reg = get_areg(s, reg0);
         return gen_lea_indexed(env, s, reg);
     case 7: /* Other */
-        switch (insn & 7) {
+        switch (reg0) {
         case 0: /* Absolute short.  */
             offset = (int16_t)read_im16(env, s);
             return tcg_const_i32(offset);
@@ -749,39 +749,26 @@ static TCGv gen_lea(CPUM68KState *env, DisasContext *s, uint16_t insn,
     return NULL_QREG;
 }
 
-/* Helper function for gen_ea. Reuse the computed address between the
-   for read/write operands.  */
-static inline TCGv gen_ea_once(CPUM68KState *env, DisasContext *s,
-                               uint16_t insn, int opsize, TCGv val,
-                               TCGv *addrp, ea_what what)
+static TCGv gen_lea(CPUM68KState *env, DisasContext *s, uint16_t insn,
+                    int opsize)
 {
-    TCGv tmp;
-
-    if (addrp && what == EA_STORE) {
-        tmp = *addrp;
-    } else {
-        tmp = gen_lea(env, s, insn, opsize);
-        if (IS_NULL_QREG(tmp))
-            return tmp;
-        if (addrp)
-            *addrp = tmp;
-    }
-    return gen_ldst(s, opsize, tmp, val, what);
+    int mode = extract32(insn, 3, 3);
+    int reg0 = REG(insn, 0);
+    return gen_lea_mode(env, s, mode, reg0, opsize);
 }
 
-/* Generate code to load/store a value from/into an EA.  If VAL > 0 this is
+/* Generate code to load/store a value from/into an EA.  If WHAT > 0 this is
    a write otherwise it is a read (0 == sign extend, -1 == zero extend).
    ADDRP is non-null for readwrite operands.  */
-static TCGv gen_ea(CPUM68KState *env, DisasContext *s, uint16_t insn,
-                   int opsize, TCGv val, TCGv *addrp, ea_what what)
+static TCGv gen_ea_mode(CPUM68KState *env, DisasContext *s, int mode, int reg0,
+                        int opsize, TCGv val, TCGv *addrp, ea_what what)
 {
-    TCGv reg;
-    TCGv result;
-    uint32_t offset;
+    TCGv reg, tmp, result;
+    int32_t offset;
 
-    switch ((insn >> 3) & 7) {
+    switch (mode) {
     case 0: /* Data register direct.  */
-        reg = DREG(insn, 0);
+        reg = cpu_dregs[reg0];
         if (what == EA_STORE) {
             gen_partset_reg(opsize, reg, val);
             return store_dummy;
@@ -789,7 +776,7 @@ static TCGv gen_ea(CPUM68KState *env, DisasContext *s, uint16_t insn,
             return gen_extend(reg, opsize, what == EA_LOADS);
         }
     case 1: /* Address register direct.  */
-        reg = AREG(insn, 0);
+        reg = get_areg(s, reg0);
         if (what == EA_STORE) {
             tcg_gen_mov_i32(reg, val);
             return store_dummy;
@@ -797,45 +784,56 @@ static TCGv gen_ea(CPUM68KState *env, DisasContext *s, uint16_t insn,
             return gen_extend(reg, opsize, what == EA_LOADS);
         }
     case 2: /* Indirect register */
-        reg = AREG(insn, 0);
+        reg = get_areg(s, reg0);
         return gen_ldst(s, opsize, reg, val, what);
     case 3: /* Indirect postincrement.  */
-        reg = AREG(insn, 0);
+        reg = get_areg(s, reg0);
         result = gen_ldst(s, opsize, reg, val, what);
         if (what == EA_STORE || !addrp) {
             TCGv tmp = tcg_temp_new();
             tcg_gen_addi_i32(tmp, reg, opsize_bytes(opsize));
-            delay_set_areg(s, REG(insn, 0), tmp, true);
+            delay_set_areg(s, reg0, tmp, true);
         }
         return result;
     case 4: /* Indirect predecrememnt.  */
-        {
-            TCGv tmp;
-            if (addrp && what == EA_STORE) {
-                tmp = *addrp;
-            } else {
-                tmp = gen_lea(env, s, insn, opsize);
-                if (IS_NULL_QREG(tmp))
-                    return tmp;
-                if (addrp)
-                    *addrp = tmp;
+        if (addrp && what == EA_STORE) {
+            tmp = *addrp;
+        } else {
+            tmp = gen_lea_mode(env, s, mode, reg0, opsize);
+            if (IS_NULL_QREG(tmp)) {
+                return tmp;
             }
-            result = gen_ldst(s, opsize, tmp, val, what);
-            if (what == EA_STORE || !addrp) {
-                delay_set_areg(s, REG(insn, 0), tmp, false);
+            if (addrp) {
+                *addrp = tmp;
             }
         }
+        result = gen_ldst(s, opsize, tmp, val, what);
+        if (what == EA_STORE || !addrp) {
+            delay_set_areg(s, reg0, tmp, false);
+        }
         return result;
     case 5: /* Indirect displacement.  */
     case 6: /* Indirect index + displacement.  */
-        return gen_ea_once(env, s, insn, opsize, val, addrp, what);
+    do_indirect:
+        if (addrp && what == EA_STORE) {
+            tmp = *addrp;
+        } else {
+            tmp = gen_lea_mode(env, s, mode, reg0, opsize);
+            if (IS_NULL_QREG(tmp)) {
+                return tmp;
+            }
+            if (addrp) {
+                *addrp = tmp;
+            }
+        }
+        return gen_ldst(s, opsize, tmp, val, what);
     case 7: /* Other */
-        switch (insn & 7) {
+        switch (reg0) {
         case 0: /* Absolute short.  */
         case 1: /* Absolute long.  */
         case 2: /* pc displacement  */
         case 3: /* pc index+displacement.  */
-            return gen_ea_once(env, s, insn, opsize, val, addrp, what);
+            goto do_indirect;
         case 4: /* Immediate.  */
             /* Sign extend values for consistency.  */
             switch (opsize) {
@@ -868,6 +866,14 @@ static TCGv gen_ea(CPUM68KState *env, DisasContext *s, uint16_t insn,
     return NULL_QREG;
 }
 
+static TCGv gen_ea(CPUM68KState *env, DisasContext *s, uint16_t insn,
+                   int opsize, TCGv val, TCGv *addrp, ea_what what)
+{
+    int mode = extract32(insn, 3, 3);
+    int reg0 = REG(insn, 0);
+    return gen_ea_mode(env, s, mode, reg0, opsize, val, addrp, what);
+}
+
 typedef struct {
     TCGCond tcond;
     bool g1;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 3/4] target-m68k: add cmpm
  2016-11-01 21:29 [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Richard Henderson
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 1/4] target-m68k: Delay autoinc writeback Richard Henderson
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 2/4] target-m68k: Split gen_lea and gen_ea Richard Henderson
@ 2016-11-01 21:29 ` Richard Henderson
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 4/4] target-m68k: Use gen_ea_mode in cmpm Richard Henderson
  2016-11-02 12:07 ` [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Laurent Vivier
  4 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2016-11-01 21:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: laurent

From: Laurent Vivier <laurent@vivier.eu>

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <rth@twiddle.net>
Message-Id: <1477604609-2206-2-git-send-email-laurent@vivier.eu>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-m68k/translate.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 00018b4..85cdba5 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2224,6 +2224,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;
@@ -3465,6 +3492,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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 4/4] target-m68k: Use gen_ea_mode in cmpm
  2016-11-01 21:29 [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Richard Henderson
                   ` (2 preceding siblings ...)
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 3/4] target-m68k: add cmpm Richard Henderson
@ 2016-11-01 21:29 ` Richard Henderson
  2016-11-02 13:04   ` Laurent Vivier
  2016-11-02 12:07 ` [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Laurent Vivier
  4 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2016-11-01 21:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: laurent

The writeback infrastructure takes care of ensuring that Ax == Ay
both uses the updated register value from Ay for Ax, and updating
the final register result twice.

??? Maybe squash into previous.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-m68k/translate.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 85cdba5..aefd90c 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2227,26 +2227,14 @@ DISAS_INSN(cmpa)
 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);
+    TCGv src, dst;
+
+    /* Post-increment load (mode 3) from Ay.  */
+    src = gen_ea_mode(env, s, 3, REG(insn, 0), opsize,
+                      NULL_QREG, NULL, EA_LOADS);
+    /* Post-increment load (mode 3) from Ax.  */
+    dst = gen_ea_mode(env, s, 3, REG(insn, 9), opsize,
+                      NULL_QREG, NULL, EA_LOADS);
 
     gen_update_cc_cmp(s, dst, src, opsize);
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/4] target-m68k areg writeback
  2016-11-01 21:29 [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Richard Henderson
                   ` (3 preceding siblings ...)
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 4/4] target-m68k: Use gen_ea_mode in cmpm Richard Henderson
@ 2016-11-02 12:07 ` Laurent Vivier
  2016-11-02 15:25   ` Richard Henderson
  4 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2016-11-02 12:07 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel



Le 01/11/2016 à 22:29, Richard Henderson a écrit :
> Here's the patch I almost wrote in the email, followed by
> a cleanup that allows cmpm to be written "nicely".
> 
> I can test this to some extent with the coldfire kernel,
> but of course coldfire can't excersise any of the tricky
> edge cases that m68000 can.
> 
> I'm particularly interested in edge cases like
> 
> 	mov.b	a0@+, a0@+
> 	movea	a0@+, a0
> 	movea	a0, a0@-
> 
> The first two are not really useful and likely not show up
> in normal code.  The third may well do so; I think our
> current code gets it wrong, but this will get it right.

Checked on a real 68040:


stack:
        .long 0x3456789a
top:

        .text
        .globl _start
_start:
        lea top,%a0
        move.l %a0,-(%a0)

The result is:
    %a0 = top - 4
and top is stored in place of "0x3456789a".
[this is also what happens without your patches]

If I try this with you patch, I have a core dump:

m68k-linux-gnu-gcc -g -m68040 -nostartfiles  -nodefaultlibs  -nostdlib
-o move move.S

./m68k-linux-user/qemu-m68k -singlestep -d in_asm,cpu -cpu m68040
../qemu-m68k/tests/m68k/move

----------------
IN:
0x800000b8:  lea 0x800020d0,%a0

D0 = 00000000   A0 = 00000000   F0 = 0000000000000000 (           0)
D1 = 00000000   A1 = 00000000   F1 = 0000000000000000 (           0)
D2 = 00000000   A2 = 00000000   F2 = 0000000000000000 (           0)
D3 = 00000000   A3 = 00000000   F3 = 0000000000000000 (           0)
D4 = 00000000   A4 = 00000000   F4 = 0000000000000000 (           0)
D5 = 00000000   A5 = 00000000   F5 = 0000000000000000 (           0)
D6 = 00000000   A6 = 00000000   F6 = 0000000000000000 (           0)
D7 = 00000000   A7 = f6fff0a0   F7 = 0000000000000000 (           0)
PC = 800000b8   SR = 0000 ----- FPRESULT =            0
----------------
IN:
0x800000be:  movel %a0,%a0@-

D0 = 00000000   A0 = 800020d0   F0 = 0000000000000000 (           0)
D1 = 00000000   A1 = 00000000   F1 = 0000000000000000 (           0)
D2 = 00000000   A2 = 00000000   F2 = 0000000000000000 (           0)
D3 = 00000000   A3 = 00000000   F3 = 0000000000000000 (           0)
D4 = 00000000   A4 = 00000000   F4 = 0000000000000000 (           0)
D5 = 00000000   A5 = 00000000   F5 = 0000000000000000 (           0)
D6 = 00000000   A6 = 00000000   F6 = 0000000000000000 (           0)
D7 = 00000000   A7 = f6fff0a0   F7 = 0000000000000000 (           0)
PC = 800000be   SR = 0000 ----- FPRESULT =            0
qemu-m68k: tcg/tcg.c:653: tcg_temp_free_internal: Assertion `idx >=
s->nb_globals && idx < s->nb_temps' failed.
qemu-m68k: translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.

Laurent

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 4/4] target-m68k: Use gen_ea_mode in cmpm
  2016-11-01 21:29 ` [Qemu-devel] [PATCH 4/4] target-m68k: Use gen_ea_mode in cmpm Richard Henderson
@ 2016-11-02 13:04   ` Laurent Vivier
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2016-11-02 13:04 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel



Le 01/11/2016 à 22:29, Richard Henderson a écrit :
> The writeback infrastructure takes care of ensuring that Ax == Ay
> both uses the updated register value from Ay for Ax, and updating
> the final register result twice.
> 
> ??? Maybe squash into previous.

I think the previous one should be completely removed and replaced by
this one.

Thanks,
Laurent

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/4] target-m68k areg writeback
  2016-11-02 12:07 ` [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Laurent Vivier
@ 2016-11-02 15:25   ` Richard Henderson
  2016-11-02 15:32     ` Laurent Vivier
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2016-11-02 15:25 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel

On 11/02/2016 06:07 AM, Laurent Vivier wrote:
> qemu-m68k: tcg/tcg.c:653: tcg_temp_free_internal: Assertion `idx >=
> s->nb_globals && idx < s->nb_temps' failed.
> qemu-m68k: translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.

Bah.  Forgot to clear the new data to begin.


r~


diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index aefd90c..2e85ca9 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3543,6 +3543,7 @@ void gen_intermediate_code
      dc->fpcr = env->fpcr;
      dc->user = (env->sr & SR_S) == 0;
      dc->done_mac = 0;
+    dc->writeback_mask = 0;
      num_insns = 0;
      max_insns = tb->cflags & CF_COUNT_MASK;
      if (max_insns == 0) {

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/4] target-m68k areg writeback
  2016-11-02 15:25   ` Richard Henderson
@ 2016-11-02 15:32     ` Laurent Vivier
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2016-11-02 15:32 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel



Le 02/11/2016 à 16:25, Richard Henderson a écrit :
> On 11/02/2016 06:07 AM, Laurent Vivier wrote:
>> qemu-m68k: tcg/tcg.c:653: tcg_temp_free_internal: Assertion `idx >=
>> s->nb_globals && idx < s->nb_temps' failed.
>> qemu-m68k: translate-all.c:175: tb_lock: Assertion `!have_tb_lock'
>> failed.
> 
> Bah.  Forgot to clear the new data to begin.

It works fine with this fix.

Thanks,
Laurent

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-11-02 15:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-01 21:29 [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Richard Henderson
2016-11-01 21:29 ` [Qemu-devel] [PATCH 1/4] target-m68k: Delay autoinc writeback Richard Henderson
2016-11-01 21:29 ` [Qemu-devel] [PATCH 2/4] target-m68k: Split gen_lea and gen_ea Richard Henderson
2016-11-01 21:29 ` [Qemu-devel] [PATCH 3/4] target-m68k: add cmpm Richard Henderson
2016-11-01 21:29 ` [Qemu-devel] [PATCH 4/4] target-m68k: Use gen_ea_mode in cmpm Richard Henderson
2016-11-02 13:04   ` Laurent Vivier
2016-11-02 12:07 ` [Qemu-devel] [PATCH 0/4] target-m68k areg writeback Laurent Vivier
2016-11-02 15:25   ` Richard Henderson
2016-11-02 15:32     ` Laurent Vivier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.