[Qemu-devel] [PATCH] tcg: Fix tcg_reg_alloc_mov vs no-op truncation

[Qemu-devel] [PATCH] tcg: Fix tcg_reg_alloc_mov vs no-op truncation

[Richard Henderson]

Commit af3cbfbe8018ccc16fb3a0048e928f66f0d05e87 hoisted some "common"
loads of the temporary type, forgetting that the types could differ
during truncating moves.  This affects the correctness of the memory
offset on big-endian hosts.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/tcg.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 0670aff..ea8aa70 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2096,12 +2096,15 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
 {
     TCGRegSet allocated_regs;
     TCGTemp *ts, *ots;
-    TCGType type;
+    TCGType otype, itype;
 
     tcg_regset_set(allocated_regs, s->reserved_regs);
     ots = &s->temps[args[0]];
     ts = &s->temps[args[1]];
-    type = ots->type;
+
+    /* Note that otype != itype for no-op truncation.  */
+    otype = ots->type;
+    itype = ts->type;
 
     /* If the source value is not in a register, and we're going to be
        forced to have it in a register in order to perform the copy,
@@ -2109,13 +2112,13 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
        we don't have to reload SOURCE the next time it is used. */
     if (((NEED_SYNC_ARG(0) || ots->fixed_reg) && ts->val_type != TEMP_VAL_REG)
         || ts->val_type == TEMP_VAL_MEM) {
-        ts->reg = tcg_reg_alloc(s, tcg_target_available_regs[type],
+        ts->reg = tcg_reg_alloc(s, tcg_target_available_regs[itype],
                                 allocated_regs);
         if (ts->val_type == TEMP_VAL_MEM) {
-            tcg_out_ld(s, type, ts->reg, ts->mem_reg, ts->mem_offset);
+            tcg_out_ld(s, itype, ts->reg, ts->mem_reg, ts->mem_offset);
             ts->mem_coherent = 1;
         } else if (ts->val_type == TEMP_VAL_CONST) {
-            tcg_out_movi(s, type, ts->reg, ts->val);
+            tcg_out_movi(s, itype, ts->reg, ts->val);
         }
         s->reg_to_temp[ts->reg] = args[1];
         ts->val_type = TEMP_VAL_REG;
@@ -2130,7 +2133,7 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
         if (!ots->mem_allocated) {
             temp_allocate_frame(s, args[0]);
         }
-        tcg_out_st(s, type, ts->reg, ots->mem_reg, ots->mem_offset);
+        tcg_out_st(s, otype, ts->reg, ots->mem_reg, ots->mem_offset);
         if (IS_DEAD_ARG(1)) {
             temp_dead(s, args[1]);
         }
@@ -2158,10 +2161,10 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
                 /* When allocating a new register, make sure to not spill the
                    input one. */
                 tcg_regset_set_reg(allocated_regs, ts->reg);
-                ots->reg = tcg_reg_alloc(s, tcg_target_available_regs[type],
+                ots->reg = tcg_reg_alloc(s, tcg_target_available_regs[otype],
                                          allocated_regs);
             }
-            tcg_out_mov(s, type, ots->reg, ts->reg);
+            tcg_out_mov(s, otype, ots->reg, ts->reg);
         }
         ots->val_type = TEMP_VAL_REG;
         ots->mem_coherent = 0;
-- 
1.9.0

Re: [Qemu-devel] [PATCH] tcg: Fix tcg_reg_alloc_mov vs no-op truncation

[Tom Musta]

On 5/13/2014 4:59 PM, Richard Henderson wrote:
> Commit af3cbfbe8018ccc16fb3a0048e928f66f0d05e87 hoisted some "common"
> loads of the temporary type, forgetting that the types could differ
> during truncating moves.  This affects the correctness of the memory
> offset on big-endian hosts.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Tested-by: Tom Musta <tommusta@gmail.com>

Works functionally and also the low level traces look better:

---

IN:
0x00000fff79d62ae4:  rlwimi  r4,r4,8,16,23

OP:
 ld_i32 tmp0,env,$0xfffffffffffffffc
 movi_i32 tmp1,$0x0
 brcond_i32 tmp0,tmp1,ne,$0x0
 ---- 0xfff79d62ae4
 mov_i32 tmp0,r4
 movi_i32 tmp1,$0x8
 rotl_i32 tmp0,tmp0,tmp1
 movi_i64 tmp3,$0xffffffff
 and_i64 tmp2,tmp0,tmp3
 movi_i64 tmp4,$0xff00
 and_i64 tmp2,tmp2,tmp4
 movi_i64 tmp4,$0xffffffffffff00ff
 and_i64 tmp3,r4,tmp4
 or_i64 r4,tmp2,tmp3
 goto_tb $0x0
 movi_i64 nip,$0xfff79d62ae8
 exit_tb $0xfff79e7dc30
 set_label $0x0
 exit_tb $0xfff79e7dc33

OUT: [size=136]
0x60363920:  lwz     r14,-4(r27)
0x60363924:  cmpwi   cr7,r14,0
0x60363928:  bne-    cr7,0x60363994
0x6036392c:  ld      r14,32(r27)  # TEM -- looks better
0x60363930:  mr      r15,r14
0x60363934:  rotlwi  r15,r15,8
0x60363938:  andi.   r15,r15,65280
0x6036393c:  lis     r0,-1
0x60363940:  ori     r0,r0,255
0x60363944:  and     r14,r14,r0
0x60363948:  or      r14,r15,r14
0x6036394c:  std     r14,32(r27)
0x60363950:  .long 0x0
0x60363954:  .long 0x0
0x60363958:  .long 0x0
...
NIP 00000fff79d62ae8   LR 00000fff79d55af0 CTR 0000000000000007 XER 0000000000000000
MSR 8000000002006000 HID0 0000000000000000  HF 0000000002006000 idx 0
TB 00000000 00000000
GPR00 00000000100002d9 0000004000a0e040 00000fff79d88ae8 00000000100002d9
GPR04 0000000000002424 0000000000000000 0000000000000000 0000000000000000        # TEM so does GPR04
GPR08 0000000000000000 0000000010010978 000000007fffffff 0000000000000001