target-arm: Fix garbage collection of temporaries in Neon emulation.

+
+    /* gen_helper_neon_mull_[su]{8|16} do not free their parameters.
+       Don't forget to clean them now.  */
+    switch ((size << 1) | u) {
+    case 0:

+    case 1:
+    case 2:
+    case 3:
+      dead_tmp(a);
+      dead_tmp(b);
+      break;
+    }
 }

Review

Quoted 13 lines · Christophe Lyon

(quoted diff) +12 -0

+
+    /* gen_helper_neon_mull_[su]{8|16} do not free their parameters.
+       Don't forget to clean them now.  */
+    switch ((size << 1) | u) {
+    case 0:
+    case 1:
+    case 2:
+    case 3:
+      dead_tmp(a);
+      dead_tmp(b);
+      break;
+    }
 }

This seems a rather convoluted way to write "if (size < 2) { ... }"

Quoted 9 lines

(quoted diff) +4 -0

@@ -5235,9 +5245,12 @@ static int disas_neon_data_insn(CPUState * env,
 DisasContext *s, uint32_t insn)
                            tmp = neon_load_reg(rn, 0);
                        } else {
                            tmp = tmp3;
+                           /* tmp2 has been discarded in
+                              gen_neon_mull during pass 0, we need to
+                              recreate it.  */
+                           tmp2 = neon_get_scalar(size, rm);

Review static int disas_neon_data_insn(CPUState * env, · line 5253

Quoted 1 line

(quoted diff) +0 -0

                        }

I think this will give the wrong results for instructions where the scalar operand is in the same Neon register as the destination for the first pass, because calling neon_get_scalar() again will do a reload from the Neon register and it might have changed. (Also loading it once at the start rather than in every pass is more efficient as well as being correct :-))

Also your patch has hard-coded tabs in it (please see CODING_STYLE on the subject of whitespace) and your mail client or server has line-wrapped long lines in the patch so it doesn't apply cleanly...

Here is an updated patch which will hopefully not be mangled by my mailer.

Fix garbage collection of temporaries in Neon emulation.

I would personally prefer slightly less terse commit messages (for instance it might be nice to list the affected instructions in this case). The convention is also to preface the summary line with the file or directory affected, ie "target-arm: Fix garbage collection of temporaries in Neon emulation".

Here is an updated patch which will hopefully not be mangled by my mailer.

Fix garbage collection of temporaries in Neon emulation.

Signed-off-by Christophe Lyon <[email protected]>

---
 target-arm/translate.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

target-arm/translate.c +13 -5

index 57664bc..b3e3d70 100644
@@ -4176,6 +4176,13 @@ static inline void gen_neon_mull(TCGv_i64 dest, TCGv a, TCGv b, int size, int u)
         break;
     default: abort();
     }
+
+    /* gen_helper_neon_mull_[su]{8|16} do not free their parameters.
+       Don't forget to clean them now.  */
+    if (size < 2) {
+      dead_tmp(a);
+      dead_tmp(b);
+    }
 }
 
 /* Translate a NEON data processing instruction.  Return nonzero if the

@@ -4840,7 +4847,7 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 if (size == 3) {
                     tcg_temp_free_i64(tmp64);
                 } else {
-                    dead_tmp(tmp2);
+                    tcg_temp_free_i32(tmp2);
                 }
             } else if (op == 10) {
                 /* VSHLL */

@@ -5076,8 +5083,6 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                     case 8: case 9: case 10: case 11: case 12: case 13:
                         /* VMLAL, VQDMLAL, VMLSL, VQDMLSL, VMULL, VQDMULL */
                         gen_neon_mull(cpu_V0, tmp, tmp2, size, u);
-                        dead_tmp(tmp2);
-                        dead_tmp(tmp);
                         break;
                     case 14: /* Polynomial VMULL */
                         cpu_abort(env, "Polynomial VMULL not implemented");

@@ -5228,6 +5233,10 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                         return 1;
 
                     tmp2 = neon_get_scalar(size, rm);
+                    /* We need a copy of tmp2 because gen_neon_mull
+                     * deletes it during pass 0.  */
+                    tmp4 = new_tmp();
+                    tcg_gen_mov_i32(tmp4, tmp2);
                     tmp3 = neon_load_reg(rn, 1);
 
                     for (pass = 0; pass < 2; pass++) {

@@ -5235,9 +5244,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                             tmp = neon_load_reg(rn, 0);
                         } else {
                             tmp = tmp3;
+                            tmp2 = tmp4;
                         }
                         gen_neon_mull(cpu_V0, tmp, tmp2, size, u);
-                        dead_tmp(tmp);
                         if (op == 6 || op == 7) {
                             gen_neon_negl(cpu_V0, size);
                         }

@@ -5264,7 +5273,6 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                         neon_store_reg64(cpu_V0, rd + pass);
                     }
 
-                    dead_tmp(tmp2);
 
                     break;
                 default: /* 14 and 15 are RESERVED */

Signature · 1 line

1.7.2.3

Incidentally there are some correctness fixes for the multiply-by-scalar neon insns from the qemu-meego tree which are on my list to push upstream. So you probably aren't getting the right results even if you've managed to shut up qemu's warnings :-)

Quoted 4 lines · Peter Maydell

Incidentally there are some correctness fixes for the multiply-by-scalar neon insns from the qemu-meego tree which are on my list to push upstream. So you probably aren't getting the right results even if you've managed to shut up qemu's warnings :-)

And yes I can confirm there are many other wrong results in the Neon support, which I am currently fixing.