Re: [Qemu-devel] [PATCH] target-arm: Fix garbage collection of temporaries in Neon emulation.

[Peter Maydell]

On 18 January 2011 14:34, Christophe Lyon <address@hidden> wrote:
> +
> +    /* gen_helper_neon_mull_[su]{8|16} do not free their parameters.
> +       Don't forget to clean them now.  */
> +    switch ((size << 1) | u) {
> +    case 0:
> +    case 1:
> +    case 2:
> +    case 3:
> +      dead_tmp(a);
> +      dead_tmp(b);
> +      break;
> +    }
>  }

This seems a rather convoluted way to write "if (size < 2) { ... }"

> @@ -5235,9 +5245,12 @@ static int disas_neon_data_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
>                             tmp = neon_load_reg(rn, 0);
>                         } else {
>                             tmp = tmp3;
> +                           /* tmp2 has been discarded in
> +                              gen_neon_mull during pass 0, we need to
> +                              recreate it.  */
> +                           tmp2 = neon_get_scalar(size, rm);
>                         }

I think this will give the wrong results for instructions where the
scalar operand is in the same Neon register as the destination
for the first pass, because calling neon_get_scalar() again will
do a reload from the Neon register and it might have changed.
(Also loading it once at the start rather than in every pass is
more efficient as well as being correct :-))

Also your patch has hard-coded tabs in it (please see
CODING_STYLE on the subject of whitespace) and your
mail client or server has line-wrapped long lines in the patch
so it doesn't apply cleanly...

-- PMM