[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target-arm: tcg temp variable usage cleanup
From: |
Laurent Desnogues |
Subject: |
Re: [Qemu-devel] [PATCH] target-arm: tcg temp variable usage cleanup |
Date: |
Sun, 1 Nov 2009 01:08:59 +0100 |
On Thu, Oct 29, 2009 at 3:01 PM, <address@hidden> wrote:
> From: Juha Riihimäki <address@hidden>
>
> 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 <address@hidden>
> ---
> 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