Re: [Qemu-devel] [PATCH v2 06/10] target-arm: fix neon vsri, vshl and vsli ops

[Laurent Desnogues]

On Sat, Oct 24, 2009 at 2:19 PM,  <address@hidden> wrote:
> From: Juha Riihimäki <address@hidden>
>
> Shift by immediate value is incorrectly overwritten by a temporary
> variable in the processing of NEON vsri, vshl and vsli instructions.
> This patch has been revised to also include a fix for the special
> case where the code would previously try to shift an integer value
> over 31 bits left/right.
>
> Signed-off-by: Juha Riihimäki <address@hidden>

Acked-by: Laurent Desnogues <address@hidden>


Laurent

> ---
>  target-arm/translate.c |   32 ++++++++++++++++++--------------
>  1 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index d1e2ed2..9e924d4 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4098,7 +4098,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, mask;
>     TCGv tmp, tmp2, tmp3, tmp4, tmp5;
>     TCGv_i64 tmp64;
>
> @@ -4626,31 +4626,35 @@ static int disas_neon_data_insn(CPUState * env, 
> DisasContext *s, uint32_t insn)
>                             switch (size) {
>                             case 0:
>                                 if (op == 4)
> -                                    imm = 0xff >> -shift;
> +                                    mask = 0xff >> -shift;
>                                 else
> -                                    imm = (uint8_t)(0xff << shift);
> -                                imm |= imm << 8;
> -                                imm |= imm << 16;
> +                                    mask = (uint8_t)(0xff << shift);
> +                                mask |= mask << 8;
> +                                mask |= mask << 16;
>                                 break;
>                             case 1:
>                                 if (op == 4)
> -                                    imm = 0xffff >> -shift;
> +                                    mask = 0xffff >> -shift;
>                                 else
> -                                    imm = (uint16_t)(0xffff << shift);
> -                                imm |= imm << 16;
> +                                    mask = (uint16_t)(0xffff << shift);
> +                                mask |= mask << 16;
>                                 break;
>                             case 2:
> -                                if (op == 4)
> -                                    imm = 0xffffffffu >> -shift;
> -                                else
> -                                    imm = 0xffffffffu << shift;
> +                                if (shift < -31 || shift > 31) {
> +                                    mask = 0;
> +                                } else {
> +                                    if (op == 4)
> +                                        mask = 0xffffffffu >> -shift;
> +                                    else
> +                                        mask = 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, mask);
> +                            tcg_gen_andi_i32(tmp2, tmp2, ~mask);
>                             tcg_gen_or_i32(tmp, tmp, tmp2);
>                             dead_tmp(tmp2);
>                         }
> --
> 1.6.5
>
>
>
>