[Qemu-devel] [PATCH v2] target-mips: Fix ALIGN instruction when bp=0

* [Qemu-devel] [PATCH v2] target-mips: Fix ALIGN instruction when bp=0
@ 2016-01-04 15:52 Miodrag Dinic
  2016-01-04 18:01 ` P J P
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Miodrag Dinic @ 2016-01-04 15:52 UTC (permalink / raw)
  To: Aurelien Jarno; +Cc: Miodrag Dinic, qemu-devel, Petar Jovanovic

[-- Attachment #1: Type: text/plain, Size: 3459 bytes --]

Hello Aurelien,

thanks for your comments and review.
Version 2 of the patch is in the attachment.

Diff between versions 1 & 2 according to your comments is :

diff --git a/target-mips/translate.c b/target-mips/translate.c
index f20678c..d2443d3 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -4632,12 +4632,13 @@ static void gen_align(DisasContext *ctx, int opc, int rd, int rs, int rt,
     if (bp == 0) {
         switch (opc) {
         case OPC_ALIGN:
+            tcg_gen_ext32s_tl(cpu_gpr[rd], t0);
+            break;
 #if defined(TARGET_MIPS64)
-            tcg_gen_ext32s_i64(cpu_gpr[rd], t0);
+        case OPC_DALIGN:
+            tcg_gen_mov_tl(cpu_gpr[rd], t0);
             break;
 #endif
-        default:
-            tcg_gen_mov_tl(cpu_gpr[rd], t0);
         }
     } else {
         TCGv t1 = tcg_temp_new();

* As you suggested I used tcg_gen_ext32s_tl() instead of tcg_gen_ext32s_i64() for the OPC_ALIGN case.

* I've kept the "TARGET_MIPS64" ifdef guard for the OPC_DALIGN case, to keep the change in-line with the rest of the code where this 64-bit instruction opcode is used.

Thank you.

Regards,
Miodrag

________________________________________
From: Aurelien Jarno [aurelien@aurel32.net]
Sent: Friday, January 01, 2016 2:02 PM
To: Miodrag Dinic
Cc: qemu-devel@nongnu.org; Petar Jovanovic
Subject: Re: [PATCH] target-mips: Fix ALIGN instruction when bp=0

[snip]

> From e01539a11061c447bece8dccde1715da9534024d Mon Sep 17 00:00:00 2001
> From: Miodrag Dinic <miodrag.dinic@imgtec.com>
> Date: Thu, 3 Dec 2015 16:48:57 +0100
> Subject: [PATCH] target-mips: Fix ALIGN instruction when bp=0
>
> If executing ALIGN with shift count bp=0 within mips64 emulation,
> the result of the operation should be sign extended.
>
> Taken from the official documentation (pseudo code) :
>
> ALIGN:
>       tmp_rt_hi = unsigned_word(GPR[rt]) << (8*bp)
>       tmp_rs_lo = unsigned_word(GPR[rs]) >> (8*(4-bp))
>       tmp = tmp_rt_hi || tmp_rt_lo
>       GPR[rd] = sign_extend.32(tmp)
>
> Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
> ---
>  target-mips/translate.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 5626647..f20678c 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -4630,7 +4630,15 @@ static void gen_align(DisasContext *ctx, int opc, int rd, int rs, int rt,
>      t0 = tcg_temp_new();
>      gen_load_gpr(t0, rt);
>      if (bp == 0) {
> -        tcg_gen_mov_tl(cpu_gpr[rd], t0);
> +        switch (opc) {
> +        case OPC_ALIGN:
> +#if defined(TARGET_MIPS64)
> +            tcg_gen_ext32s_i64(cpu_gpr[rd], t0);
> +            break;
> +#endif

The way to fix that is basically ok. However you should just use
tcg_gen_ext32s_tl instead of tcg_gen_ext32s_i64 and drop the
TARGET_MIPS64 #ifdef.

> +        default:

Then you can replace this by OPC_DALIGN for more clarity.

> +            tcg_gen_mov_tl(cpu_gpr[rd], t0);
> +        }
>      } else {
>          TCGv t1 = tcg_temp_new();
>          gen_load_gpr(t1, rs);

The resulting binary code should be the same, but less #ifdef means less
risk of breakage, as the code is always compiled.

--
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-PATCH-v2-target-mips-Fix-ALIGN-instruction-when-bp-0.patch --]
[-- Type: text/x-patch; name="0001-PATCH-v2-target-mips-Fix-ALIGN-instruction-when-bp-0.patch", Size: 1434 bytes --]

From e48787a25de9a04985226cd7651795403d5752e6 Mon Sep 17 00:00:00 2001
From: Miodrag Dinic <miodrag.dinic@imgtec.com>
Date: Thu, 3 Dec 2015 16:48:57 +0100
Subject: [PATCH] [PATCH v2] target-mips: Fix ALIGN instruction when bp=0

If executing ALIGN with shift count bp=0 within mips64 emulation,
the result of the operation should be sign extended.

Taken from the official documentation (pseudo code) :

ALIGN:
	tmp_rt_hi = unsigned_word(GPR[rt]) << (8*bp)
	tmp_rs_lo = unsigned_word(GPR[rs]) >> (8*(4-bp))
	tmp = tmp_rt_hi || tmp_rt_lo
	GPR[rd] = sign_extend.32(tmp)

Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
---
 target-mips/translate.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 5626647..d2443d3 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -4630,7 +4630,16 @@ static void gen_align(DisasContext *ctx, int opc, int rd, int rs, int rt,
     t0 = tcg_temp_new();
     gen_load_gpr(t0, rt);
     if (bp == 0) {
-        tcg_gen_mov_tl(cpu_gpr[rd], t0);
+        switch (opc) {
+        case OPC_ALIGN:
+            tcg_gen_ext32s_tl(cpu_gpr[rd], t0);
+            break;
+#if defined(TARGET_MIPS64)
+        case OPC_DALIGN:
+            tcg_gen_mov_tl(cpu_gpr[rd], t0);
+            break;
+#endif
+        }
     } else {
         TCGv t1 = tcg_temp_new();
         gen_load_gpr(t1, rs);
-- 
1.9.1


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