[Qemu-devel] [PATCH] target-arm: fix UMAAL instruction

[Qemu-devel] [PATCH] target-arm: fix UMAAL instruction

[Aurelien Jarno]

UMAAL should use unsigned multiply instead of signed.

This patch fixes this issue by handling UMAAL separately from
UMULL/UMLAL/SMULL/SUMLAL are these instructions are different
enough. It also explicitely list instructions in case and catch
inexistant instruction as illegal. Also fixes a few style issues.

This fixes the issues reported in
https://bugs.launchpad.net/qemu/+bug/696015

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-arm/translate.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 8d494ec..eb207a8 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6637,26 +6637,38 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
                             gen_logic_CC(tmp);
                         store_reg(s, rd, tmp);
                         break;
-                    default:
+                    case 4:
+                        /* 64 bit mul double accumulate */
+                        ARCH(6);
+                        tmp = load_reg(s, rs);
+                        tmp2 = load_reg(s, rm);
+                        tmp64 = gen_mulu_i64_i32(tmp, tmp2);
+                        gen_addq_lo(s, tmp64, rn);
+                        gen_addq_lo(s, tmp64, rd);
+                        gen_storeq_reg(s, rn, rd, tmp64);
+                        tcg_temp_free_i64(tmp64);
+                        break;
+                    case 8: case 9: case 10: case 11:
+                    case 12: case 13: case 14: case 15:
                         /* 64 bit mul */
                         tmp = load_reg(s, rs);
                         tmp2 = load_reg(s, rm);
-                        if (insn & (1 << 22))
+                        if (insn & (1 << 22)) {
                             tmp64 = gen_muls_i64_i32(tmp, tmp2);
-                        else
+                        } else {
                             tmp64 = gen_mulu_i64_i32(tmp, tmp2);
-                        if (insn & (1 << 21)) /* mult accumulate */
+                        }
+                        if (insn & (1 << 21)) { /* mult accumulate */
                             gen_addq(s, tmp64, rn, rd);
-                        if (!(insn & (1 << 23))) { /* double accumulate */
-                            ARCH(6);
-                            gen_addq_lo(s, tmp64, rn);
-                            gen_addq_lo(s, tmp64, rd);
                         }
-                        if (insn & (1 << 20))
+                        if (insn & (1 << 20)) {
                             gen_logicq_cc(tmp64);
+                        }
                         gen_storeq_reg(s, rn, rd, tmp64);
                         tcg_temp_free_i64(tmp64);
                         break;
+                    default:
+                        goto illegal_op;
                     }
                 } else {
                     rn = (insn >> 16) & 0xf;
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] target-arm: fix UMAAL instruction

[Peter Maydell]

On 31 December 2010 19:11, Aurelien Jarno <aurelien@aurel32.net> wrote:
> UMAAL should use unsigned multiply instead of signed.

Looks like we've both been working on the same thing this
evening :-)

> This patch fixes this issue by handling UMAAL separately from
> UMULL/UMLAL/SMULL/SUMLAL are these instructions are different

"SMLAL as".

> It also explicitely list instructions in case and catch
> inexistant instruction as illegal.

"explicitly" and "nonexistent", if you're tweaking the commit msg
anyway.

(If you want to use the ARM ARM terminology these instruction
patterns are "UNDEFINED" rather than "illegal".)

> +                    case 4:
> +                        /* 64 bit mul double accumulate */

Can we list the instruction mnemonic in the comment too?
(Saves future readers having to wade through the decode logic...)
This case is UMAAL.

> +                    case 8: case 9: case 10: case 11:
> +                    case 12: case 13: case 14: case 15:

/* UMULL, UMLAL, SMULL, SMLAL. */

I've run the patch through my random-sequence test and it passes.

-- PMM