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?
Regards,
Juha
