| Message ID | EBC968B2-03EB-450B-B7F6-B5619F24E390@nokia.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 19, 2009 at 1:44 PM, <Juha.Riihimaki@nokia.com> wrote: > Current ARM translator code has several places where it leaves > temporary TCG variables alive. This patch removes all such instances I > have found so far. Sorry for the mangled inlined patch, the mailserver > I use doesn't like patches so I've also included it as an attachment > that hopefully status correctly formatted. Should apply cleanly > against latest git. Here are my comments. A lack of comment means I agree :-) > Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com> > --- > diff --git a/target-arm/translate.c b/target-arm/translate.c > index e56082b..bc51bcb 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -287,6 +287,7 @@ static TCGv_i64 gen_mulu_i64_i32(TCGv a, TCGv b) > tcg_gen_extu_i32_i64(tmp2, b); > dead_tmp(b); > tcg_gen_mul_i64(tmp1, tmp1, tmp2); > + tcg_temp_free_i64(tmp2); > return tmp1; > } > > @@ -300,6 +301,7 @@ static TCGv_i64 gen_muls_i64_i32(TCGv a, TCGv b) > tcg_gen_ext_i32_i64(tmp2, b); > dead_tmp(b); > tcg_gen_mul_i64(tmp1, tmp1, tmp2); > + tcg_temp_free_i64(tmp2); > return tmp1; > } > > @@ -312,9 +314,11 @@ static void gen_mull(TCGv a, TCGv b) > tcg_gen_extu_i32_i64(tmp1, a); > tcg_gen_extu_i32_i64(tmp2, b); > tcg_gen_mul_i64(tmp1, tmp1, tmp2); > + tcg_temp_free_i64(tmp2); > tcg_gen_trunc_i64_i32(a, tmp1); > tcg_gen_shri_i64(tmp1, tmp1, 32); > tcg_gen_trunc_i64_i32(b, tmp1); > + tcg_temp_free_i64(tmp1); > } > > /* Signed 32x32->64 multiply. */ > @@ -326,9 +330,11 @@ static void gen_imull(TCGv a, TCGv b) > tcg_gen_ext_i32_i64(tmp1, a); > tcg_gen_ext_i32_i64(tmp2, b); > tcg_gen_mul_i64(tmp1, tmp1, tmp2); > + tcg_temp_free_i64(tmp2); > tcg_gen_trunc_i64_i32(a, tmp1); > tcg_gen_shri_i64(tmp1, tmp1, 32); > tcg_gen_trunc_i64_i32(b, tmp1); > + tcg_temp_free_i64(tmp1); > } > > /* Swap low and high halfwords. */ > @@ -542,11 +548,13 @@ static void gen_arm_parallel_addsub(int op1, int > op2, TCGv a, TCGv b) > tmp = tcg_temp_new_ptr(); > tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); > PAS_OP(s) > + tcg_temp_free_ptr(tmp); > break; > case 5: > tmp = tcg_temp_new_ptr(); > tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); > PAS_OP(u) > + tcg_temp_free_ptr(tmp); > break; > #undef gen_pas_helper > #define gen_pas_helper(name) glue(gen_helper_,name)(a, a, b) > @@ -587,11 +595,13 @@ static void gen_thumb2_parallel_addsub(int op1, > int op2, TCGv a, TCGv b) > tmp = tcg_temp_new_ptr(); > tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); > PAS_OP(s) > + tcg_temp_free_ptr(tmp); > break; > case 4: > tmp = tcg_temp_new_ptr(); > tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); > PAS_OP(u) > + tcg_temp_free_ptr(tmp); > break; > #undef gen_pas_helper > #define gen_pas_helper(name) glue(gen_helper_,name)(a, a, b) > @@ -995,10 +1005,12 @@ static inline void gen_vfp_tosiz(int dp) > #define VFP_GEN_FIX(name) \ > static inline void gen_vfp_##name(int dp, int shift) \ > { \ > + TCGv tmp_shift = tcg_const_i32(shift); \ > if (dp) \ > - gen_helper_vfp_##name##d(cpu_F0d, cpu_F0d, tcg_const_i32 > (shift), cpu_env);\ > + gen_helper_vfp_##name##d(cpu_F0d, cpu_F0d, tmp_shift, > cpu_env);\ > else \ > - gen_helper_vfp_##name##s(cpu_F0s, cpu_F0s, tcg_const_i32 > (shift), cpu_env);\ > + gen_helper_vfp_##name##s(cpu_F0s, cpu_F0s, tmp_shift, > cpu_env);\ > + tcg_temp_free_i32(tmp_shift); \ > } > VFP_GEN_FIX(tosh) > VFP_GEN_FIX(tosl) > @@ -2399,7 +2411,7 @@ static int disas_dsp_insn(CPUState *env, > DisasContext *s, uint32_t insn) > instruction is not defined. */ > static int disas_cp_insn(CPUState *env, DisasContext *s, uint32_t > insn) > { > - TCGv tmp; > + TCGv tmp, tmp2; > uint32_t rd = (insn >> 12) & 0xf; > uint32_t cp = (insn >> 8) & 0xf; > if (IS_USER(s)) { > @@ -2411,14 +2423,18 @@ static int disas_cp_insn(CPUState *env, > DisasContext *s, uint32_t insn) > return 1; > gen_set_pc_im(s->pc); > tmp = new_tmp(); > - gen_helper_get_cp(tmp, cpu_env, tcg_const_i32(insn)); > + tmp2 = tcg_const_i32(insn); > + gen_helper_get_cp(tmp, cpu_env, tmp2); > + tcg_temp_free(tmp2); > store_reg(s, rd, tmp); > } else { > if (!env->cp[cp].cp_write) > return 1; > gen_set_pc_im(s->pc); > tmp = load_reg(s, rd); > - gen_helper_set_cp(cpu_env, tcg_const_i32(insn), tmp); > + tmp2 = tcg_const_i32(insn); > + gen_helper_set_cp(cpu_env, tmp2, tmp); > + tcg_temp_free(tmp2); > dead_tmp(tmp); > } > return 0; > @@ -2449,7 +2465,7 @@ static int cp15_user_ok(uint32_t insn) > static int disas_cp15_insn(CPUState *env, DisasContext *s, uint32_t > insn) > { > uint32_t rd; > - TCGv tmp; > + TCGv tmp, tmp2; > > /* M profile cores use memory mapped registers instead of cp15. > */ > if (arm_feature(env, ARM_FEATURE_M)) > @@ -2478,9 +2494,10 @@ static int disas_cp15_insn(CPUState *env, > DisasContext *s, uint32_t insn) > return 0; > } > rd = (insn >> 12) & 0xf; > + tmp2 = tcg_const_i32(insn); > if (insn & ARM_CP_RW_BIT) { > tmp = new_tmp(); > - gen_helper_get_cp15(tmp, cpu_env, tcg_const_i32(insn)); > + gen_helper_get_cp15(tmp, cpu_env, tmp2); > /* If the destination register is r15 then sets condition > codes. */ > if (rd != 15) > store_reg(s, rd, tmp); > @@ -2488,7 +2505,7 @@ static int disas_cp15_insn(CPUState *env, > DisasContext *s, uint32_t insn) > dead_tmp(tmp); > } else { > tmp = load_reg(s, rd); > - gen_helper_set_cp15(cpu_env, tcg_const_i32(insn), tmp); > + gen_helper_set_cp15(cpu_env, tmp2, tmp); > dead_tmp(tmp); > /* Normally we would always end the TB here, but Linux > * arch/arm/mach-pxa/sleep.S expects two instructions > following > @@ -2497,6 +2514,7 @@ static int disas_cp15_insn(CPUState *env, > DisasContext *s, uint32_t insn) > (insn & 0x0fff0fff) != 0x0e010f10) > gen_lookup_tb(s); > } > + tcg_temp_free_i32(tmp2); > return 0; > } > > @@ -4676,12 +4694,16 @@ static int disas_neon_data_insn(CPUState * > env, DisasContext *s, uint32_t insn) > gen_neon_narrow_satu(size - 1, tmp, > cpu_V0); > } > if (pass == 0) { > + dead_tmp(tmp2); This looks wrong if size == 3 since we have TCGV_UNUSED(tmp2). > tmp2 = tmp; > } else { > neon_store_reg(rd, 0, tmp2); > neon_store_reg(rd, 1, tmp); > } > } /* for pass */ > + if (size == 3) { > + tcg_temp_free_i64(tmp64); > + } > } else if (op == 10) { > /* VSHLL */ > if (q || size == 3) > @@ -5147,6 +5169,7 @@ static int disas_neon_data_insn(CPUState * env, > DisasContext *s, uint32_t insn) > tcg_gen_shli_i64(cpu_V1, cpu_V1, 64 - (imm * 8)); > tcg_gen_shri_i64(tmp64, tmp64, imm * 8); > tcg_gen_or_i64(cpu_V1, cpu_V1, tmp64); > + tcg_temp_free_i64(tmp64); > } else { > /* BUGFIX */ > neon_load_reg64(cpu_V0, rn); > @@ -5511,8 +5534,9 @@ static int disas_neon_data_insn(CPUState * env, > DisasContext *s, uint32_t insn) > tcg_gen_movi_i32(tmp, 0); > } > tmp2 = neon_load_reg(rm, 0); > - gen_helper_neon_tbl(tmp2, tmp2, tmp, tcg_const_i32(rn), > - tcg_const_i32(n)); > + TCGv tmp_rn = tcg_const_i32(rn); > + TCGv tmp_n = tcg_const_i32(n); I don't know what QEMU coding rules say about declarations in the middle of the code, but I don't like them too much :-) > + gen_helper_neon_tbl(tmp2, tmp2, tmp, tmp_rn, tmp_n); > dead_tmp(tmp); > if (insn & (1 << 6)) { > tmp = neon_load_reg(rd, 1); > @@ -5521,8 +5545,9 @@ static int disas_neon_data_insn(CPUState * env, > DisasContext *s, uint32_t insn) > tcg_gen_movi_i32(tmp, 0); > } > tmp3 = neon_load_reg(rm, 1); > - gen_helper_neon_tbl(tmp3, tmp3, tmp, tcg_const_i32(rn), > - tcg_const_i32(n)); > + gen_helper_neon_tbl(tmp3, tmp3, tmp, tmp_rn, tmp_n); > + dead_tmp(tmp_n); > + dead_tmp(tmp_rn); > neon_store_reg(rd, 0, tmp2); > neon_store_reg(rd, 1, tmp3); > dead_tmp(tmp); > @@ -5660,7 +5685,7 @@ static int disas_coproc_insn(CPUState * env, > DisasContext *s, uint32_t insn) > } > > > -/* Store a 64-bit value to a register pair. Clobbers val. */ > +/* Store a 64-bit value to a register pair. Marks val dead. */ > static void gen_storeq_reg(DisasContext *s, int rlow, int rhigh, > TCGv_i64 val) > { > TCGv tmp; > @@ -5671,6 +5696,7 @@ static void gen_storeq_reg(DisasContext *s, int > rlow, int rhigh, TCGv_i64 val) > tcg_gen_shri_i64(val, val, 32); > tcg_gen_trunc_i64_i32(tmp, val); > store_reg(s, rhigh, tmp); > + tcg_temp_free_i64(val); I'd prefer to see val freed after calls to gen_storeq_reg even if it means a longer patch. The current situation with regards to implicit temp new and free is already messy enough. > } > > /* load a 32-bit value from a register and perform a 64-bit > accumulate. */ > @@ -5685,6 +5711,7 @@ static void gen_addq_lo(DisasContext *s, > TCGv_i64 val, int rlow) > tcg_gen_extu_i32_i64(tmp, tmp2); > dead_tmp(tmp2); > tcg_gen_add_i64(val, val, tmp); > + tcg_temp_free_i64(tmp); > } > > /* load and add a 64-bit value from a register pair. */ > @@ -5702,6 +5729,7 @@ static void gen_addq(DisasContext *s, TCGv_i64 > val, int rlow, int rhigh) > dead_tmp(tmpl); > dead_tmp(tmph); > tcg_gen_add_i64(val, val, tmp); > + tcg_temp_free_i64(tmp); > } > > /* Set N and Z flags from a 64-bit value. */ > @@ -5785,7 +5813,9 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > addr = load_reg(s, 13); > } else { > addr = new_tmp(); > - gen_helper_get_r13_banked(addr, cpu_env, tcg_const_i32 > (op1)); > + TCGv tmp_op1 = tcg_const_i32(op1); No declaration in code (see above). > + gen_helper_get_r13_banked(addr, cpu_env, tmp_op1); > + tcg_temp_free_i32(tmp_op1); > } > i = (insn >> 23) & 3; > switch (i) { > @@ -5816,7 +5846,9 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > if (op1 == (env->uncached_cpsr & CPSR_M)) { > store_reg(s, 13, addr); > } else { > - gen_helper_set_r13_banked(cpu_env, tcg_const_i32 > (op1), addr); > + TCGv tmp_op1 = tcg_const_i32(op1); Same as above. > + gen_helper_set_r13_banked(cpu_env, tmp_op1, addr); > + tcg_temp_free_i32(tmp_op1); > dead_tmp(addr); > } > } else { > @@ -6058,6 +6090,7 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > tcg_gen_shri_i64(tmp64, tmp64, 16); > tmp = new_tmp(); > tcg_gen_trunc_i64_i32(tmp, tmp64); > + tcg_temp_free_i64(tmp64); > if ((sh & 2) == 0) { > tmp2 = load_reg(s, rn); > gen_helper_add_setq(tmp, tmp, tmp2); > @@ -6545,10 +6578,12 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > } > sh = (insn >> 16) & 0x1f; > if (sh != 0) { > + TCGv tmp_sh = tcg_const_i32(sh); > if (insn & (1 << 22)) > - gen_helper_usat(tmp, tmp, > tcg_const_i32(sh)); > + gen_helper_usat(tmp, tmp, tmp_sh); > else > - gen_helper_ssat(tmp, tmp, > tcg_const_i32(sh)); > + gen_helper_ssat(tmp, tmp, tmp_sh); > + tcg_temp_free_i32(tmp_sh); > } > store_reg(s, rd, tmp); > } else if ((insn & 0x00300fe0) == 0x00200f20) { > @@ -6556,10 +6591,12 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > tmp = load_reg(s, rm); > sh = (insn >> 16) & 0x1f; > if (sh != 0) { > + TCGv tmp_sh = tcg_const_i32(sh); > if (insn & (1 << 22)) > - gen_helper_usat16(tmp, tmp, > tcg_const_i32(sh)); > + gen_helper_usat16(tmp, tmp, tmp_sh); > else > - gen_helper_ssat16(tmp, tmp, > tcg_const_i32(sh)); > + gen_helper_ssat16(tmp, tmp, tmp_sh); > + tcg_temp_free_i32(tmp_sh); > } > store_reg(s, rd, tmp); > } else if ((insn & 0x00700fe0) == 0x00000fa0) { > @@ -6631,6 +6668,7 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > tcg_gen_shri_i64(tmp64, tmp64, 32); > tmp = new_tmp(); > tcg_gen_trunc_i64_i32(tmp, tmp64); > + tcg_temp_free_i64(tmp64); > if (rd != 15) { > tmp2 = load_reg(s, rd); > if (insn & (1 << 6)) { > @@ -6831,7 +6869,9 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > if (i == 15) { > gen_bx(s, tmp); > } else if (user) { > - gen_helper_set_user_reg(tcg_const_i32 > (i), tmp); > + TCGv tmp_i = tcg_const_i32(i); > + gen_helper_set_user_reg(tmp_i, tmp); > + tcg_temp_free_i32(tmp_i); > dead_tmp(tmp); > } else if (i == rn) { > loaded_var = tmp; > @@ -6848,7 +6888,9 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > tcg_gen_movi_i32(tmp, val); > } else if (user) { > tmp = new_tmp(); > - gen_helper_get_user_reg(tmp, > tcg_const_i32(i)); > + TCGv tmp_i = tcg_const_i32(i); Same as above. > + gen_helper_get_user_reg(tmp, tmp_i); > + tcg_temp_free_i32(tmp_i); > } else { > tmp = load_reg(s, i); > } > @@ -7264,7 +7306,9 @@ static int disas_thumb2_insn(CPUState *env, > DisasContext *s, uint16_t insn_hw1) > addr = load_reg(s, 13); > } else { > addr = new_tmp(); > - gen_helper_get_r13_banked(addr, cpu_env, > tcg_const_i32(op)); > + TCGv tmp_op = tcg_const_i32(op); Same as above. > + gen_helper_get_r13_banked(addr, cpu_env, > tmp_op); > + tcg_temp_free_i32(tmp_op); > } > if ((insn & (1 << 24)) == 0) { > tcg_gen_addi_i32(addr, addr, -8); > @@ -7284,8 +7328,9 @@ static int disas_thumb2_insn(CPUState *env, > DisasContext *s, uint16_t insn_hw1) > if (op == (env->uncached_cpsr & CPSR_M)) { > store_reg(s, 13, addr); > } else { > - gen_helper_set_r13_banked(cpu_env, > - tcg_const_i32(op), addr); > + TCGv tmp_op = tcg_const_i32(op); > + gen_helper_set_r13_banked(cpu_env, > tmp_op, addr); > + tcg_temp_free_i32(tmp_op); > } > } else { > dead_tmp(addr); > @@ -7515,6 +7560,7 @@ static int disas_thumb2_insn(CPUState *env, > DisasContext *s, uint16_t insn_hw1) > tcg_gen_shri_i64(tmp64, tmp64, 16); > tmp = new_tmp(); > tcg_gen_trunc_i64_i32(tmp, tmp64); > + tcg_temp_free_i64(tmp64); > if (rs != 15) > { > tmp2 = load_reg(s, rs); > @@ -7673,6 +7719,7 @@ static int disas_thumb2_insn(CPUState *env, > DisasContext *s, uint16_t insn_hw1) > tmp = load_reg(s, rn); > addr = tcg_const_i32(insn & 0xff); > gen_helper_v7m_msr(cpu_env, addr, tmp); > + tcg_temp_free_i32(addr); Shouldn't tmp be freed too? > gen_lookup_tb(s); > break; > } > @@ -7742,6 +7789,7 @@ static int disas_thumb2_insn(CPUState *env, > DisasContext *s, uint16_t insn_hw1) > if (IS_M(env)) { > addr = tcg_const_i32(insn & 0xff); > gen_helper_v7m_mrs(tmp, cpu_env, addr); > + tcg_temp_free_i32(addr); > } else { > gen_helper_cpsr_read(tmp); > } > @@ -7842,6 +7890,7 @@ static int disas_thumb2_insn(CPUState *env, > DisasContext *s, uint16_t insn_hw1) > else > gen_helper_ssat(tmp, tmp, tmp2); > } > + tcg_temp_free_i32(tmp2); > break; > } > store_reg(s, rd, tmp); > @@ -8614,12 +8663,15 @@ static void disas_thumb_insn(CPUState *env, > DisasContext *s) > if (insn & 1) { > addr = tcg_const_i32(16); > gen_helper_v7m_msr(cpu_env, addr, tmp); > + tcg_temp_free_i32(addr); > } > /* FAULTMASK */ > if (insn & 2) { > addr = tcg_const_i32(17); > gen_helper_v7m_msr(cpu_env, addr, tmp); > + tcg_temp_free_i32(addr); > } > + tcg_temp_free_i32(tmp); > gen_lookup_tb(s); > } else { > if (insn & (1 << 4)) >
Thanks for your feedback. I'll iron out the issues you mentioned, please look at my further inlined comments below. Cheers, Juha On Oct 21, 2009, at 22:20, ext Laurent Desnogues wrote: >> @@ -4676,12 +4694,16 @@ static int disas_neon_data_insn(CPUState * >> env, DisasContext *s, uint32_t insn) >> gen_neon_narrow_satu(size - 1, tmp, >> cpu_V0); >> } >> if (pass == 0) { >> + dead_tmp(tmp2); > > This looks wrong if size == 3 since we have TCGV_UNUSED(tmp2). You're right. However, looking at the surrounding code a bit closer I began to wonder if it works correctly at all since tmp2 is used as a shift value if size < 2 but it is also used to store the result of the first pass. Therefore the result of the first pass is used as a shift value during second pass... perhaps not correct? Wouldn't it make more sense to store the result of the first pass in the lower half of the destination neon register directly during the first pass? I see no point in keeping it in a temporary variable and only store both halves of the destination neon register during the second pass. Especially since there is no memory access involved, everything is done in registers. What do you think? >> @@ -5511,8 +5534,9 @@ static int disas_neon_data_insn(CPUState * env, >> DisasContext *s, uint32_t insn) >> tcg_gen_movi_i32(tmp, 0); >> } >> tmp2 = neon_load_reg(rm, 0); >> - gen_helper_neon_tbl(tmp2, tmp2, tmp, tcg_const_i32 >> (rn), >> - tcg_const_i32(n)); >> + TCGv tmp_rn = tcg_const_i32(rn); >> + TCGv tmp_n = tcg_const_i32(n); > > I don't know what QEMU coding rules say about declarations in > the middle of the code, but I don't like them too much :-) Ah, sorry. I forgot to clean up my own cleanups ;) I'll fix this and the other instances you noticed. >> @@ -5671,6 +5696,7 @@ static void gen_storeq_reg(DisasContext *s, int >> rlow, int rhigh, TCGv_i64 val) >> tcg_gen_shri_i64(val, val, 32); >> tcg_gen_trunc_i64_i32(tmp, val); >> store_reg(s, rhigh, tmp); >> + tcg_temp_free_i64(val); > > I'd prefer to see val freed after calls to gen_storeq_reg even > if it means a longer patch. The current situation with regards > to implicit temp new and free is already messy enough. Certainly. I guess the whole file could be patched to remove all such implicit allocations and deallocations and make everything explicit; that should clear things up and make it more readable. I could do that as well. I implemented this change to make it behave like the other store_reg functions in the target-arm/translate.c, I thought having methods with similar functionality behaving differently in regards to the arguments would make things even messier. >> @@ -7673,6 +7719,7 @@ static int disas_thumb2_insn(CPUState *env, >> DisasContext *s, uint16_t insn_hw1) >> tmp = load_reg(s, rn); >> addr = tcg_const_i32(insn & 0xff); >> gen_helper_v7m_msr(cpu_env, addr, tmp); >> + tcg_temp_free_i32(addr); > > Shouldn't tmp be freed too? Correct. I'll add that.
diff --git a/target-arm/translate.c b/target-arm/translate.c index e56082b..bc51bcb 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -287,6 +287,7 @@ static TCGv_i64 gen_mulu_i64_i32(TCGv a, TCGv b) tcg_gen_extu_i32_i64(tmp2, b); dead_tmp(b); tcg_gen_mul_i64(tmp1, tmp1, tmp2); + tcg_temp_free_i64(tmp2); return tmp1; } @@ -300,6 +301,7 @@ static TCGv_i64 gen_muls_i64_i32(TCGv a, TCGv b) tcg_gen_ext_i32_i64(tmp2, b); dead_tmp(b); tcg_gen_mul_i64(tmp1, tmp1, tmp2); + tcg_temp_free_i64(tmp2); return tmp1; } @@ -312,9 +314,11 @@ static void gen_mull(TCGv a, TCGv b) tcg_gen_extu_i32_i64(tmp1, a); tcg_gen_extu_i32_i64(tmp2, b); tcg_gen_mul_i64(tmp1, tmp1, tmp2); + tcg_temp_free_i64(tmp2); tcg_gen_trunc_i64_i32(a, tmp1); tcg_gen_shri_i64(tmp1, tmp1, 32); tcg_gen_trunc_i64_i32(b, tmp1); + tcg_temp_free_i64(tmp1); } /* Signed 32x32->64 multiply. */ @@ -326,9 +330,11 @@ static void gen_imull(TCGv a, TCGv b) tcg_gen_ext_i32_i64(tmp1, a); tcg_gen_ext_i32_i64(tmp2, b); tcg_gen_mul_i64(tmp1, tmp1, tmp2); + tcg_temp_free_i64(tmp2); tcg_gen_trunc_i64_i32(a, tmp1); tcg_gen_shri_i64(tmp1, tmp1, 32); tcg_gen_trunc_i64_i32(b, tmp1); + tcg_temp_free_i64(tmp1); } /* Swap low and high halfwords. */ @@ -542,11 +548,13 @@ static void gen_arm_parallel_addsub(int op1, int op2, TCGv a, TCGv b) tmp = tcg_temp_new_ptr(); tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); PAS_OP(s) + tcg_temp_free_ptr(tmp); break; case 5: tmp = tcg_temp_new_ptr(); tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); PAS_OP(u) + tcg_temp_free_ptr(tmp); break; #undef gen_pas_helper
Current ARM translator code has several places where it leaves temporary TCG variables alive. This patch removes all such instances I have found so far. Sorry for the mangled inlined patch, the mailserver I use doesn't like patches so I've also included it as an attachment that hopefully status correctly formatted. Should apply cleanly against latest git. Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com> --- #define gen_pas_helper(name) glue(gen_helper_,name)(a, a, b) @@ -587,11 +595,13 @@ static void gen_thumb2_parallel_addsub(int op1, int op2, TCGv a, TCGv b) tmp = tcg_temp_new_ptr(); tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); PAS_OP(s) + tcg_temp_free_ptr(tmp); break; case 4: tmp = tcg_temp_new_ptr(); tcg_gen_addi_ptr(tmp, cpu_env, offsetof(CPUState, GE)); PAS_OP(u) + tcg_temp_free_ptr(tmp); break; #undef gen_pas_helper #define gen_pas_helper(name) glue(gen_helper_,name)(a, a, b) @@ -995,10 +1005,12 @@ static inline void gen_vfp_tosiz(int dp) #define VFP_GEN_FIX(name) \ static inline void gen_vfp_##name(int dp, int shift) \ { \ + TCGv tmp_shift = tcg_const_i32(shift); \ if (dp) \ - gen_helper_vfp_##name##d(cpu_F0d, cpu_F0d, tcg_const_i32 (shift), cpu_env);\ + gen_helper_vfp_##name##d(cpu_F0d, cpu_F0d, tmp_shift, cpu_env);\ else \ - gen_helper_vfp_##name##s(cpu_F0s, cpu_F0s, tcg_const_i32 (shift), cpu_env);\ + gen_helper_vfp_##name##s(cpu_F0s, cpu_F0s, tmp_shift, cpu_env);\ + tcg_temp_free_i32(tmp_shift); \ } VFP_GEN_FIX(tosh) VFP_GEN_FIX(tosl) @@ -2399,7 +2411,7 @@ static int disas_dsp_insn(CPUState *env, DisasContext *s, uint32_t insn) instruction is not defined. */ static int disas_cp_insn(CPUState *env, DisasContext *s, uint32_t insn) { - TCGv tmp; + TCGv tmp, tmp2; uint32_t rd = (insn >> 12) & 0xf; uint32_t cp = (insn >> 8) & 0xf; if (IS_USER(s)) { @@ -2411,14 +2423,18 @@ static int disas_cp_insn(CPUState *env, DisasContext *s, uint32_t insn) return 1; gen_set_pc_im(s->pc); tmp = new_tmp(); - gen_helper_get_cp(tmp, cpu_env, tcg_const_i32(insn)); + tmp2 = tcg_const_i32(insn); + gen_helper_get_cp(tmp, cpu_env, tmp2); + tcg_temp_free(tmp2); store_reg(s, rd, tmp); } else { if (!env->cp[cp].cp_write) return 1; gen_set_pc_im(s->pc); tmp = load_reg(s, rd); - gen_helper_set_cp(cpu_env, tcg_const_i32(insn), tmp); + tmp2 = tcg_const_i32(insn); + gen_helper_set_cp(cpu_env, tmp2, tmp); + tcg_temp_free(tmp2); dead_tmp(tmp); } return 0; @@ -2449,7 +2465,7 @@ static int cp15_user_ok(uint32_t insn) static int disas_cp15_insn(CPUState *env, DisasContext *s, uint32_t insn) { uint32_t rd; - TCGv tmp; + TCGv tmp, tmp2; /* M profile cores use memory mapped registers instead of cp15. */ if (arm_feature(env, ARM_FEATURE_M)) @@ -2478,9 +2494,10 @@ static int disas_cp15_insn(CPUState *env, DisasContext *s, uint32_t insn) return 0; } rd = (insn >> 12) & 0xf; + tmp2 = tcg_const_i32(insn); if (insn & ARM_CP_RW_BIT) { tmp = new_tmp(); - gen_helper_get_cp15(tmp, cpu_env, tcg_const_i32(insn)); + gen_helper_get_cp15(tmp, cpu_env, tmp2); /* If the destination register is r15 then sets condition codes. */ if (rd != 15) store_reg(s, rd, tmp); @@ -2488,7 +2505,7 @@ static int disas_cp15_insn(CPUState *env, DisasContext *s, uint32_t insn) dead_tmp(tmp); } else { tmp = load_reg(s, rd); - gen_helper_set_cp15(cpu_env, tcg_const_i32(insn), tmp); + gen_helper_set_cp15(cpu_env, tmp2, tmp); dead_tmp(tmp); /* Normally we would always end the TB here, but Linux * arch/arm/mach-pxa/sleep.S expects two instructions following @@ -2497,6 +2514,7 @@ static int disas_cp15_insn(CPUState *env, DisasContext *s, uint32_t insn) (insn & 0x0fff0fff) != 0x0e010f10) gen_lookup_tb(s); } + tcg_temp_free_i32(tmp2); return 0; } @@ -4676,12 +4694,16 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn) gen_neon_narrow_satu(size - 1, tmp, cpu_V0); } if (pass == 0) { + dead_tmp(tmp2); tmp2 = tmp; } else { neon_store_reg(rd, 0, tmp2); neon_store_reg(rd, 1, tmp); } } /* for pass */ + if (size == 3) { + tcg_temp_free_i64(tmp64); + } } else if (op == 10) { /* VSHLL */ if (q || size == 3) @@ -5147,6 +5169,7 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn) tcg_gen_shli_i64(cpu_V1, cpu_V1, 64 - (imm * 8)); tcg_gen_shri_i64(tmp64, tmp64, imm * 8); tcg_gen_or_i64(cpu_V1, cpu_V1, tmp64); + tcg_temp_free_i64(tmp64); } else { /* BUGFIX */ neon_load_reg64(cpu_V0, rn); @@ -5511,8 +5534,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn) tcg_gen_movi_i32(tmp, 0); } tmp2 = neon_load_reg(rm, 0); - gen_helper_neon_tbl(tmp2, tmp2, tmp, tcg_const_i32(rn), - tcg_const_i32(n)); + TCGv tmp_rn = tcg_const_i32(rn); + TCGv tmp_n = tcg_const_i32(n); + gen_helper_neon_tbl(tmp2, tmp2, tmp, tmp_rn, tmp_n); dead_tmp(tmp); if (insn & (1 << 6)) { tmp = neon_load_reg(rd, 1); @@ -5521,8 +5545,9 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn) tcg_gen_movi_i32(tmp, 0); } tmp3 = neon_load_reg(rm, 1); - gen_helper_neon_tbl(tmp3, tmp3, tmp, tcg_const_i32(rn), - tcg_const_i32(n)); + gen_helper_neon_tbl(tmp3, tmp3, tmp, tmp_rn, tmp_n); + dead_tmp(tmp_n); + dead_tmp(tmp_rn); neon_store_reg(rd, 0, tmp2); neon_store_reg(rd, 1, tmp3); dead_tmp(tmp); @@ -5660,7 +5685,7 @@ static int disas_coproc_insn(CPUState * env, DisasContext *s, uint32_t insn) } -/* Store a 64-bit value to a register pair. Clobbers val. */ +/* Store a 64-bit value to a register pair. Marks val dead. */ static void gen_storeq_reg(DisasContext *s, int rlow, int rhigh, TCGv_i64 val) { TCGv tmp; @@ -5671,6 +5696,7 @@ static void gen_storeq_reg(DisasContext *s, int rlow, int rhigh, TCGv_i64 val) tcg_gen_shri_i64(val, val, 32); tcg_gen_trunc_i64_i32(tmp, val); store_reg(s, rhigh, tmp); + tcg_temp_free_i64(val); } /* load a 32-bit value from a register and perform a 64-bit accumulate. */ @@ -5685,6 +5711,7 @@ static void gen_addq_lo(DisasContext *s, TCGv_i64 val, int rlow) tcg_gen_extu_i32_i64(tmp, tmp2); dead_tmp(tmp2); tcg_gen_add_i64(val, val, tmp); + tcg_temp_free_i64(tmp); } /* load and add a 64-bit value from a register pair. */ @@ -5702,6 +5729,7 @@ static void gen_addq(DisasContext *s, TCGv_i64 val, int rlow, int rhigh) dead_tmp(tmpl); dead_tmp(tmph); tcg_gen_add_i64(val, val, tmp); + tcg_temp_free_i64(tmp); } /* Set N and Z flags from a 64-bit value. */ @@ -5785,7 +5813,9 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) addr = load_reg(s, 13); } else { addr = new_tmp(); - gen_helper_get_r13_banked(addr, cpu_env, tcg_const_i32 (op1)); + TCGv tmp_op1 = tcg_const_i32(op1); + gen_helper_get_r13_banked(addr, cpu_env, tmp_op1); + tcg_temp_free_i32(tmp_op1); } i = (insn >> 23) & 3; switch (i) { @@ -5816,7 +5846,9 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) if (op1 == (env->uncached_cpsr & CPSR_M)) { store_reg(s, 13, addr); } else { - gen_helper_set_r13_banked(cpu_env, tcg_const_i32 (op1), addr); + TCGv tmp_op1 = tcg_const_i32(op1); + gen_helper_set_r13_banked(cpu_env, tmp_op1, addr); + tcg_temp_free_i32(tmp_op1); dead_tmp(addr); } } else { @@ -6058,6 +6090,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) tcg_gen_shri_i64(tmp64, tmp64, 16); tmp = new_tmp(); tcg_gen_trunc_i64_i32(tmp, tmp64); + tcg_temp_free_i64(tmp64); if ((sh & 2) == 0) { tmp2 = load_reg(s, rn); gen_helper_add_setq(tmp, tmp, tmp2); @@ -6545,10 +6578,12 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) } sh = (insn >> 16) & 0x1f; if (sh != 0) { + TCGv tmp_sh = tcg_const_i32(sh); if (insn & (1 << 22)) - gen_helper_usat(tmp, tmp, tcg_const_i32(sh)); + gen_helper_usat(tmp, tmp, tmp_sh); else - gen_helper_ssat(tmp, tmp, tcg_const_i32(sh)); + gen_helper_ssat(tmp, tmp, tmp_sh); + tcg_temp_free_i32(tmp_sh); } store_reg(s, rd, tmp); } else if ((insn & 0x00300fe0) == 0x00200f20) { @@ -6556,10 +6591,12 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) tmp = load_reg(s, rm); sh = (insn >> 16) & 0x1f; if (sh != 0) { + TCGv tmp_sh = tcg_const_i32(sh); if (insn & (1 << 22)) - gen_helper_usat16(tmp, tmp, tcg_const_i32(sh)); + gen_helper_usat16(tmp, tmp, tmp_sh); else - gen_helper_ssat16(tmp, tmp, tcg_const_i32(sh)); + gen_helper_ssat16(tmp, tmp, tmp_sh); + tcg_temp_free_i32(tmp_sh); } store_reg(s, rd, tmp); } else if ((insn & 0x00700fe0) == 0x00000fa0) { @@ -6631,6 +6668,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) tcg_gen_shri_i64(tmp64, tmp64, 32); tmp = new_tmp(); tcg_gen_trunc_i64_i32(tmp, tmp64); + tcg_temp_free_i64(tmp64); if (rd != 15) { tmp2 = load_reg(s, rd); if (insn & (1 << 6)) { @@ -6831,7 +6869,9 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) if (i == 15) { gen_bx(s, tmp); } else if (user) { - gen_helper_set_user_reg(tcg_const_i32 (i), tmp); + TCGv tmp_i = tcg_const_i32(i); + gen_helper_set_user_reg(tmp_i, tmp); + tcg_temp_free_i32(tmp_i); dead_tmp(tmp); } else if (i == rn) { loaded_var = tmp; @@ -6848,7 +6888,9 @@ static void disas_arm_insn(CPUState * env, DisasContext *s) tcg_gen_movi_i32(tmp, val); } else if (user) { tmp = new_tmp(); - gen_helper_get_user_reg(tmp, tcg_const_i32(i)); + TCGv tmp_i = tcg_const_i32(i); + gen_helper_get_user_reg(tmp, tmp_i); + tcg_temp_free_i32(tmp_i); } else { tmp = load_reg(s, i); } @@ -7264,7 +7306,9 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1) addr = load_reg(s, 13); } else { addr = new_tmp(); - gen_helper_get_r13_banked(addr, cpu_env, tcg_const_i32(op)); + TCGv tmp_op = tcg_const_i32(op); + gen_helper_get_r13_banked(addr, cpu_env, tmp_op); + tcg_temp_free_i32(tmp_op); } if ((insn & (1 << 24)) == 0) { tcg_gen_addi_i32(addr, addr, -8); @@ -7284,8 +7328,9 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1) if (op == (env->uncached_cpsr & CPSR_M)) { store_reg(s, 13, addr); } else { - gen_helper_set_r13_banked(cpu_env, - tcg_const_i32(op), addr); + TCGv tmp_op = tcg_const_i32(op); + gen_helper_set_r13_banked(cpu_env, tmp_op, addr); + tcg_temp_free_i32(tmp_op); } } else { dead_tmp(addr); @@ -7515,6 +7560,7 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1) tcg_gen_shri_i64(tmp64, tmp64, 16); tmp = new_tmp(); tcg_gen_trunc_i64_i32(tmp, tmp64); + tcg_temp_free_i64(tmp64); if (rs != 15) { tmp2 = load_reg(s, rs); @@ -7673,6 +7719,7 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1) tmp = load_reg(s, rn); addr = tcg_const_i32(insn & 0xff); gen_helper_v7m_msr(cpu_env, addr, tmp); + tcg_temp_free_i32(addr); gen_lookup_tb(s); break; } @@ -7742,6 +7789,7 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1) if (IS_M(env)) { addr = tcg_const_i32(insn & 0xff); gen_helper_v7m_mrs(tmp, cpu_env, addr); + tcg_temp_free_i32(addr); } else { gen_helper_cpsr_read(tmp); } @@ -7842,6 +7890,7 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1) else gen_helper_ssat(tmp, tmp, tmp2); } + tcg_temp_free_i32(tmp2); break; } store_reg(s, rd, tmp); @@ -8614,12 +8663,15 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s) if (insn & 1) { addr = tcg_const_i32(16); gen_helper_v7m_msr(cpu_env, addr, tmp); + tcg_temp_free_i32(addr); } /* FAULTMASK */ if (insn & 2) { addr = tcg_const_i32(17); gen_helper_v7m_msr(cpu_env, addr, tmp); + tcg_temp_free_i32(addr); } + tcg_temp_free_i32(tmp); gen_lookup_tb(s); } else { if (insn & (1 << 4))