[Qemu-devel] [PATCH 0/6] target/m68k: Optimize few instructions using deposit/extraxt_i32()

[Qemu-devel] [PATCH 0/6] target/m68k: Optimize few instructions using deposit/extraxt_i32()

[Philippe Mathieu-Daudé]

Hi Laurent, Richard.

I found these patches while cleaning dangling branches on my
previous laptop... Original commits date is 2017-07-21 09:31:32...
I simply had to rebase them.

It doesn't have to be merged for soft freeze, but since I'm doing
housekeeping I rather send it to keep archived by the ML.

Regards,

Phil.

Philippe Mathieu-Daudé (6):
  target/m68k: Reduce the l1 TCGLabel scope
  target/m68k: Optimize the partset instruction using deposit_i32()
  target/m68k: Fix a tcg_temp leak
  target/m68k: Optimize get_sr() using deposit_i32()
  target/m68k: Optimize rotate_x() using extract_i32()
  target/m68k: Reduce the scope of the 'zero' tcg_temp

 target/m68k/translate.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/6] target/m68k: Reduce the l1 TCGLabel scope

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/m68k/translate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 6217a683f1..ab801b6ceb 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -3019,7 +3019,6 @@ DISAS_INSN(branch)
     int32_t offset;
     uint32_t base;
     int op;
-    TCGLabel *l1;
 
     base = s->pc;
     op = (insn >> 8) & 0xf;
@@ -3035,7 +3034,7 @@ DISAS_INSN(branch)
     }
     if (op > 1) {
         /* Bcc */
-        l1 = gen_new_label();
+        TCGLabel *l1 = gen_new_label();
         gen_jmpcc(s, ((insn >> 8) & 0xf) ^ 1, l1);
         gen_jmp_tb(s, 1, base + offset);
         gen_set_label(l1);
-- 
2.19.1

[Qemu-devel] [PATCH 2/6] target/m68k: Optimize the partset instruction using deposit_i32()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/m68k/translate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ab801b6ceb..55766fd7ef 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -720,17 +720,15 @@ static void gen_partset_reg(int opsize, TCGv reg, TCGv val)
     TCGv tmp;
     switch (opsize) {
     case OS_BYTE:
-        tcg_gen_andi_i32(reg, reg, 0xffffff00);
         tmp = tcg_temp_new();
         tcg_gen_ext8u_i32(tmp, val);
-        tcg_gen_or_i32(reg, reg, tmp);
+        tcg_gen_deposit_i32(reg, tmp, reg, 8, 24);
         tcg_temp_free(tmp);
         break;
     case OS_WORD:
-        tcg_gen_andi_i32(reg, reg, 0xffff0000);
         tmp = tcg_temp_new();
         tcg_gen_ext16u_i32(tmp, val);
-        tcg_gen_or_i32(reg, reg, tmp);
+        tcg_gen_deposit_i32(reg, tmp, reg, 16, 16);
         tcg_temp_free(tmp);
         break;
     case OS_LONG:
-- 
2.19.1

[Qemu-devel] [PATCH 3/6] target/m68k: Fix a tcg_temp leak

[Philippe Mathieu-Daudé]

The function gen_get_ccr() returns a tcg_temp created with
tcg_temp_new(). Free it with tcg_temp_free().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Laurent/Richard, feel free to squash this with the next patch, but
IMHO having it split as a previous step makes the next patch easier
to review.
---
 target/m68k/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 55766fd7ef..ea95d55a11 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2224,6 +2224,7 @@ static TCGv gen_get_sr(DisasContext *s)
     sr = tcg_temp_new();
     tcg_gen_andi_i32(sr, QREG_SR, 0xffe0);
     tcg_gen_or_i32(sr, sr, ccr);
+    tcg_temp_free(ccr);
     return sr;
 }
 
-- 
2.19.1

[Qemu-devel] [PATCH 4/6] target/m68k: Optimize get_sr() using deposit_i32()

[Philippe Mathieu-Daudé]

Doing so we free one tcg_temp.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/m68k/translate.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ea95d55a11..f43ac07b7f 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2217,15 +2217,11 @@ static TCGv gen_get_ccr(DisasContext *s)
 
 static TCGv gen_get_sr(DisasContext *s)
 {
-    TCGv ccr;
-    TCGv sr;
+    TCGv dest;
 
-    ccr = gen_get_ccr(s);
-    sr = tcg_temp_new();
-    tcg_gen_andi_i32(sr, QREG_SR, 0xffe0);
-    tcg_gen_or_i32(sr, sr, ccr);
-    tcg_temp_free(ccr);
-    return sr;
+    dest = gen_get_ccr(s);
+    tcg_gen_deposit_i32(dest, dest, QREG_SR, 5, 11);
+    return dest;
 }
 
 static void gen_set_sr_im(DisasContext *s, uint16_t val, int ccr_only)
-- 
2.19.1

[Qemu-devel] [PATCH 5/6] target/m68k: Optimize rotate_x() using extract_i32()

[Philippe Mathieu-Daudé]

Optimize rotate_x() using tcg_gen_extract_i32(). We can now free the
'sz' tcg_temp earlier. Since it is allocated with tcg_const_i32(),
free it with tcg_temp_free_i32().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/m68k/translate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index f43ac07b7f..b51b8a2a12 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -3686,6 +3686,7 @@ static TCGv rotate_x(TCGv reg, TCGv shift, int left, int size)
         tcg_gen_sub_i32(shl, shl, shift); /* shl = size + 1 - shift */
         tcg_gen_sub_i32(shx, sz, shift); /* shx = size - shift */
     }
+    tcg_temp_free_i32(sz);
 
     /* reg = (reg << shl) | (reg >> shr) | (x << shx); */
 
@@ -3701,9 +3702,7 @@ static TCGv rotate_x(TCGv reg, TCGv shift, int left, int size)
     /* X = (reg >> size) & 1 */
 
     X = tcg_temp_new();
-    tcg_gen_shr_i32(X, reg, sz);
-    tcg_gen_andi_i32(X, X, 1);
-    tcg_temp_free(sz);
+    tcg_gen_extract_i32(X, reg, size, 1);
 
     return X;
 }
-- 
2.19.1

[Qemu-devel] [PATCH 6/6] target/m68k: Reduce the scope of the 'zero' tcg_temp

[Philippe Mathieu-Daudé]

Reduce the scope of the 'zero' tcg_temp. Since this tcg_temp is
allocated with tcg_const_i32(), free it using tcg_temp_free_i32().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/m68k/translate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index b51b8a2a12..3f27079379 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -3664,7 +3664,7 @@ static void rotate_x_flags(TCGv reg, TCGv X, int size)
 /* Result of rotate_x() is valid if 0 <= shift <= size */
 static TCGv rotate_x(TCGv reg, TCGv shift, int left, int size)
 {
-    TCGv X, shl, shr, shx, sz, zero;
+    TCGv X, shl, shr, shx, sz;
 
     sz = tcg_const_i32(size);
 
@@ -3672,14 +3672,14 @@ static TCGv rotate_x(TCGv reg, TCGv shift, int left, int size)
     shl = tcg_temp_new();
     shx = tcg_temp_new();
     if (left) {
+        TCGv zero = tcg_const_i32(0);
         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 */
         /* shx = shx < 0 ? size : shx; */
-        zero = tcg_const_i32(0);
         tcg_gen_movcond_i32(TCG_COND_LT, shx, shx, zero, sz, shx);
-        tcg_temp_free(zero);
+        tcg_temp_free_i32(zero);
     } else {
         tcg_gen_mov_i32(shr, shift);      /* shr = shift */
         tcg_gen_movi_i32(shl, size + 1);
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/6] target/m68k: Reduce the l1 TCGLabel scope

[Richard Henderson]

On 3/9/19 4:34 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/m68k/translate.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/6] target/m68k: Optimize the partset instruction using deposit_i32()

[Richard Henderson]

On 3/9/19 4:34 PM, Philippe Mathieu-Daudé wrote:
>      case OS_BYTE:
> -        tcg_gen_andi_i32(reg, reg, 0xffffff00);
>          tmp = tcg_temp_new();
>          tcg_gen_ext8u_i32(tmp, val);

Might as well elide this as well.

> -        tcg_gen_or_i32(reg, reg, tmp);
> +        tcg_gen_deposit_i32(reg, tmp, reg, 8, 24);
>          tcg_temp_free(tmp);
>          break;
>      case OS_WORD:
> -        tcg_gen_andi_i32(reg, reg, 0xffff0000);
>          tmp = tcg_temp_new();
>          tcg_gen_ext16u_i32(tmp, val);

Likewise.

> -        tcg_gen_or_i32(reg, reg, tmp);
> +        tcg_gen_deposit_i32(reg, tmp, reg, 16, 16);
>          tcg_temp_free(tmp);
>          break;

Re: [Qemu-devel] [PATCH 3/6] target/m68k: Fix a tcg_temp leak

[Richard Henderson]

On 3/9/19 4:34 PM, Philippe Mathieu-Daudé wrote:
> The function gen_get_ccr() returns a tcg_temp created with
> tcg_temp_new(). Free it with tcg_temp_free().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Laurent/Richard, feel free to squash this with the next patch, but
> IMHO having it split as a previous step makes the next patch easier
> to review.
> ---
>  target/m68k/translate.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 4/6] target/m68k: Optimize get_sr() using deposit_i32()

[Richard Henderson]

On 3/9/19 4:34 PM, Philippe Mathieu-Daudé wrote:
>  static TCGv gen_get_sr(DisasContext *s)
>  {
> -    TCGv ccr;
> -    TCGv sr;
> +    TCGv dest;
>  
> -    ccr = gen_get_ccr(s);
> -    sr = tcg_temp_new();
> -    tcg_gen_andi_i32(sr, QREG_SR, 0xffe0);
> -    tcg_gen_or_i32(sr, sr, ccr);
> -    tcg_temp_free(ccr);
> -    return sr;
> +    dest = gen_get_ccr(s);
> +    tcg_gen_deposit_i32(dest, dest, QREG_SR, 5, 11);
> +    return dest;

Err.. there's no shift of QREG_SR by 5 in the original.
I think you meant

  tcg_gen_deposit_i32(dest, QREG_SR, dest, 0, 5);

But I'd be surprised if QREG_SR even has those bits set,
and we could elide the ANDI entirely, making this just an OR.


r~

Re: [Qemu-devel] [PATCH 6/6] target/m68k: Reduce the scope of the 'zero' tcg_temp

[Richard Henderson]

On 3/9/19 4:34 PM, Philippe Mathieu-Daudé wrote:
>      if (left) {
> +        TCGv zero = tcg_const_i32(0);
>          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 */
>          /* shx = shx < 0 ? size : shx; */
> -        zero = tcg_const_i32(0);
>          tcg_gen_movcond_i32(TCG_COND_LT, shx, shx, zero, sz, shx);
> -        tcg_temp_free(zero);
> +        tcg_temp_free_i32(zero);

But you're extending its lifetime.

r~

Re: [Qemu-devel] [PATCH 5/6] target/m68k: Optimize rotate_x() using extract_i32()

[Richard Henderson]

On 3/9/19 4:34 PM, Philippe Mathieu-Daudé wrote:
> Optimize rotate_x() using tcg_gen_extract_i32(). We can now free the
> 'sz' tcg_temp earlier. Since it is allocated with tcg_const_i32(),
> free it with tcg_temp_free_i32().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/m68k/translate.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~