Re: [Qemu-devel] [PATCH] target-arm: tcg temp variable usage cleanup

[Laurent Desnogues <laurent.desnogues@gmail.com>]

On Thu, Oct 29, 2009 at 3:01 PM,  <juha.riihimaki@nokia.com> wrote:
> From: Juha Riihimäki <juha.riihimaki@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.riihimaki@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