From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1N4O0D-0003PD-S5 for qemu-devel@nongnu.org; Sat, 31 Oct 2009 20:09:01 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1N4O0D-0003OJ-DV for qemu-devel@nongnu.org; Sat, 31 Oct 2009 20:09:01 -0400 Received: from [199.232.76.173] (port=37489 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1N4O0D-0003O8-Ba for qemu-devel@nongnu.org; Sat, 31 Oct 2009 20:09:01 -0400 Received: from fg-out-1718.google.com ([72.14.220.159]:26523) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1N4O0C-0004B8-SK for qemu-devel@nongnu.org; Sat, 31 Oct 2009 20:09:01 -0400 Received: by fg-out-1718.google.com with SMTP id 16so321941fgg.10 for ; Sat, 31 Oct 2009 17:08:59 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1256824875-46345-1-git-send-email-juha.riihimaki@nokia.com> References: <1256824875-46345-1-git-send-email-juha.riihimaki@nokia.com> Date: Sun, 1 Nov 2009 01:08:59 +0100 Message-ID: <761ea48b0910311708o352ae088s81916046f2eb5ad@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH] target-arm: tcg temp variable usage cleanup From: Laurent Desnogues Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: juha.riihimaki@nokia.com Cc: qemu-devel@nongnu.org On Thu, Oct 29, 2009 at 3:01 PM, wrote: > From: Juha Riihim=E4ki > > 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 functio= n. > 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=E4ki > --- > 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, Dis= asContext *s, uint32_t insn) > TCGv_i64 tmp64; > > if (!vfp_enabled(env)) > - return 1; > + return 1; > VFP_DREG_D(rd, insn); > rn =3D (insn >> 16) & 0xf; > rm =3D insn & 0xf; > load =3D (insn & (1 << 21)) !=3D 0; > - addr =3D new_tmp(); > + addr =3D tcg_temp_new_i32(); addr should be allocated after the undefined instructions dectection. > if (load) { > TCGV_UNUSED(tmp2); > for (n =3D 0; n < 4; n++) { > - tmp =3D gen_ld8u(addr, IS_USER(s)); > + gen_ld8u(tmp, addr, IS_USER(s)); > tcg_gen_addi_i32(addr, addr, stride); > if (n =3D=3D 0) { > tmp2 =3D tmp; > + tmp =3D 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 =3D 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, Dis= asContext *s, uint32_t insn) > nregs =3D ((insn >> 8) & 3) + 1; > stride =3D (insn & (1 << 5)) ? 2 : 1; > load_reg_var(s, addr, rn); > + tmp =3D tcg_temp_new_i32(); > + tmp2 =3D tcg_temp_new_i32(); > for (reg =3D 0; reg < nregs; reg++) { > switch (size) { > case 0: > - tmp =3D gen_ld8u(addr, IS_USER(s)); > + gen_ld8u(tmp, addr, IS_USER(s)); > gen_neon_dup_u8(tmp, 0); > break; > case 1: > - tmp =3D gen_ld16u(addr, IS_USER(s)); > + gen_ld16u(tmp, addr, IS_USER(s)); > gen_neon_dup_low16(tmp); > break; > case 2: > - tmp =3D 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, DisasC= ontext *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