On Thu, Oct 29, 2009 at 3:01 PM, <juha.riihim...@nokia.com> wrote: > From: Juha Riihimäki <juha.riihim...@nokia.com> > > TCG temporary variable handling in target-arm/translate.c is currently > somewhat inconsistent; some functions allocate new temporaries that the > calling function is expected to free and some other functions free > temporaries that are passed in as parameters. This patch will remove all > such instances in the code and make the lifespan of the temporaries more > clearly visible as they are always allocated and freed within one function. > The only exception to this are the global temporaries allocated in the > beginning of the gen_intermediate_code_internal function. > > Signed-off-by: Juha Riihimäki <juha.riihim...@nokia.com> > --- > target-arm/translate.c | 2723 > ++++++++++++++++++++++++++---------------------- > 1 files changed, 1502 insertions(+), 1221 deletions(-) > > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 5784566..6982bad 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c [...] > @@ -3684,12 +3678,12 @@ static int disas_neon_ls_insn(CPUState * env, > DisasContext *s, uint32_t insn) > TCGv_i64 tmp64; > > if (!vfp_enabled(env)) > - return 1; > + return 1; > VFP_DREG_D(rd, insn); > rn = (insn >> 16) & 0xf; > rm = insn & 0xf; > load = (insn & (1 << 21)) != 0; > - addr = new_tmp(); > + addr = tcg_temp_new_i32();
addr should be allocated after the undefined instructions dectection. > if (load) { > TCGV_UNUSED(tmp2); > for (n = 0; n < 4; n++) { > - tmp = gen_ld8u(addr, IS_USER(s)); > + gen_ld8u(tmp, addr, IS_USER(s)); > tcg_gen_addi_i32(addr, addr, stride); > if (n == 0) { > tmp2 = tmp; > + tmp = tcg_temp_new_i32(); > } else { > gen_bfi(tmp2, tmp2, tmp, n * 8, 0xff); > - dead_tmp(tmp); > + tcg_temp_free_i32(tmp); > } > } > neon_store_reg(rd, pass, tmp2); > + tmp = tmp2; This is completely wrong :-) It should be rewritten as the code for store that follows. > @@ -3795,31 +3795,36 @@ static int disas_neon_ls_insn(CPUState * env, > DisasContext *s, uint32_t insn) > nregs = ((insn >> 8) & 3) + 1; > stride = (insn & (1 << 5)) ? 2 : 1; > load_reg_var(s, addr, rn); > + tmp = tcg_temp_new_i32(); > + tmp2 = tcg_temp_new_i32(); > for (reg = 0; reg < nregs; reg++) { > switch (size) { > case 0: > - tmp = gen_ld8u(addr, IS_USER(s)); > + gen_ld8u(tmp, addr, IS_USER(s)); > gen_neon_dup_u8(tmp, 0); > break; > case 1: > - tmp = gen_ld16u(addr, IS_USER(s)); > + gen_ld16u(tmp, addr, IS_USER(s)); > gen_neon_dup_low16(tmp); > break; > case 2: > - tmp = gen_ld32(addr, IS_USER(s)); > + gen_ld32(tmp, addr, IS_USER(s)); > break; > case 3: > + tcg_temp_free_i32(tmp2); > + tcg_temp_free_i32(tmp); Missing free of addr. > @@ -4184,278 +4183,304 @@ static int disas_neon_data_insn(CPUState * env, > DisasContext *s, uint32_t insn) I won't comment on this function, it lacks too many undefined detection and some instructions. > @@ -6622,23 +6782,26 @@ static void disas_arm_insn(CPUState * env, > DisasContext *s) > default: goto illegal_op; Missing free of of tmp before the goto. The rest is at least OK from a temp leak point of view. I tested your patch by running translate + TCG code gen for all of the opcodes in the range e0000000-ffffffff. For the NEON instructions I had to add correct undefined detection to let my program process the range (OTOH I didn't bother fixing the wrong decoding and/or codegen, I was just doing sanity check on your patch). Next step is to also do that for Thumb2. And then run some real programs. Laurent