On Wed, Oct 21, 2009 at 12:17 PM, <juha.riihim...@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.riihim...@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