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

[Juha.Riihimaki]

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