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

[Laurent Desnogues]

On Thu, Oct 22, 2009 at 8:49 AM,  <address@hidden> 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.


Laurent