Re: [Qemu-devel] [PATCH 10/21] target-arm: A64: Implement remaining non-pairwise int SIMD 3-reg-same insns

[Peter Maydell]

On 28 January 2014 17:13, Richard Henderson <address@hidden> wrote:
> On 01/26/2014 11:25 AM, Peter Maydell wrote:
>> @@ -6773,9 +6801,9 @@ static void disas_simd_3same_int(DisasContext *s, 
>> uint32_t insn)
>>              case 0x8: /* SSHL, USHL */
>>              {
>>                  static NeonGenTwoOpFn * const fns[3][2] = {
>> -                    { gen_helper_neon_shl_u8, gen_helper_neon_shl_s8 },
>> -                    { gen_helper_neon_shl_u16, gen_helper_neon_shl_s16 },
>> -                    { gen_helper_neon_shl_u32, gen_helper_neon_shl_s32 },
>> +                    { gen_helper_neon_shl_s8, gen_helper_neon_shl_u8 },
>> +                    { gen_helper_neon_shl_s16, gen_helper_neon_shl_u16 },
>> +                    { gen_helper_neon_shl_s32, gen_helper_neon_shl_u32 },
>>                  };
>>                  genfn = fns[size][u];
>>                  break;
>
> Oops, I clearly missed this in the review of the previous patch.
> But these bug fixes ought to be folded into the previous.

Heh, yes. I missed this too, I think I must have just
done a 'fix same bug in all these cases' edit and not
noticed some of them were preexisting from the previous
patch.

> Otherwise,
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>> +            case 0xd: /* SMIN, UMIN */
>> +            {
>> +                static NeonGenTwoOpFn * const fns[3][2] = {
>> +                    { gen_helper_neon_min_s8, gen_helper_neon_min_u8 },
>> +                    { gen_helper_neon_min_s16, gen_helper_neon_min_u16 },
>> +                    { gen_helper_neon_min_s32, gen_helper_neon_min_u32 },
>> +                };
>> +                genfn = fns[size][u];
>> +                break;
>
> Out of curiosity, what logic are you applying to using the neon helper
> functions and implementing stuff inline?
>
> It appears that you're implementing all of the 64-bit stuff inline, but using
> the existing 32-bit helpers, even if the helpers are remarkably small, like
> some of these.

My logic runs roughly "if there's a helper already, then use it
since we know it to have the correct semantics; otherwise implement
in whatever seems most straightforward". Since the A32 Neon was
generally done almost all with helpers this tends to result in the
effect you note.

thanks
-- PMM