On 28 January 2014 17:13, Richard Henderson <rth@xxxxxxxxxxx> 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 <rth@xxxxxxxxxxx>
>
>> + 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
