Re: [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops

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

On Wed, Oct 21, 2009 at 12:17 PM,  <Juha.Riihimaki@nokia.com> wrote:
> Shift immediate value is incorrectly overwritten by a temporary
> variable in the processing of NEON vsri, vshl and vsli instructions.
>
> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
> ---
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 59bf7bc..c92ecc6 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4094,7 +4094,7 @@ static int disas_neon_data_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
>      int pairwise;
>      int u;
>      int n;
> -    uint32_t imm;
> +    uint32_t imm, imm2;

I would prefer mask to imm2, but that's personal taste :-)

>      TCGv tmp;
>      TCGv tmp2;
>      TCGv tmp3;
> @@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState *
> env, DisasContext *s, uint32_t insn)
>                              switch (size) {
>                              case 0:
>                                  if (op == 4)
> -                                    imm = 0xff >> -shift;
> +                                    imm2 = 0xff >> -shift;
>                                  else
> -                                    imm = (uint8_t)(0xff << shift);
> -                                imm |= imm << 8;
> -                                imm |= imm << 16;
> +                                    imm2 = (uint8_t)(0xff << shift);
> +                                imm2 |= imm2 << 8;
> +                                imm2 |= imm2 << 16;
>                                  break;
>                              case 1:
>                                  if (op == 4)
> -                                    imm = 0xffff >> -shift;
> +                                    imm2 = 0xffff >> -shift;
>                                  else
> -                                    imm = (uint16_t)(0xffff << shift);
> -                                imm |= imm << 16;
> +                                    imm2 = (uint16_t)(0xffff << shift);
> +                                imm2 |= imm2 << 16;
>                                  break;
>                              case 2:
>                                  if (op == 4)
> -                                    imm = 0xffffffffu >> -shift;
> +                                    imm2 = 0xffffffffu >> -shift;
>                                  else
> -                                    imm = 0xffffffffu << shift;
> +                                    imm2 = 0xffffffffu << shift;
>                                  break;
>                              default:
>                                  abort();
>                              }
>                              tmp2 = neon_load_reg(rd, pass);
> -                            tcg_gen_andi_i32(tmp, tmp, imm);
> -                            tcg_gen_andi_i32(tmp2, tmp2, ~imm);
> +                            tcg_gen_andi_i32(tmp, tmp, imm2);
> +                            tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
>                              tcg_gen_or_i32(tmp, tmp, tmp2);
>                              dead_tmp(tmp2);
>                          }

I mostly agree with this, except there's a mistake already
present in the original code:  for a size of 2 the shift amount
can be 32 (look at VSRI in the ARM Architecture Reference
Manual).  Note in this case, shift will be -32.


Laurent