On Thu, Oct 22, 2009 at 9:33 AM, <juha.riihim...@nokia.com> wrote: > > On Oct 22, 2009, at 10:18, ext Laurent Desnogues wrote: > >> On Thu, Oct 22, 2009 at 8:49 AM, <juha.riihim...@nokia.com> wrote: >>> >>> On Oct 21, 2009, at 13:46, ext Laurent Desnogues wrote: >>> >>>>> @@ -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. >>> >>> True. However, I'm not sure if this causes incorrect behavior or not. >>> I'm not a NEON expert but how I understand the VSRI instruction is >>> that it will shift the element value before it is inserted in the >>> destination vector, therefore with element size 2 and shift 32 the >>> result would be always zero and I guess that would still apply if the >>> shift was -32: does it matter in which direction you shift if you >>> anyway shift all the bits out? >> >> The problem is not in the NEON semantics but rather the C >> one: the behaviour of a shift by an amount greater than or >> equal to the bit-width of the type is undefined; in this case >> imm2 is 32-bit and the shift is 32-bit. What you want is >> imm2 = 0 if shift is -32, as you correctly guessed. > > Ah, I see. Doesn't the same issue apply for the 8bit and 16bit element > sizes as well? I suppose it will be sufficient to check for a negative > shift value and in that case force the mask value to zero.
For 8- and 16-bit elements, your shift will always be <= 16 (in absolute value I mean :-), so you have no issue for these cases. Laurent