Re: [PATCH 1/2] [ARM] PR68532: Fix up vuzp for big endian

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

On 09/02/16 17:00, Charles Baylis wrote:
> On 8 February 2016 at 11:42, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>> Hi Charles,
>>
>>
>> On 03/02/16 18:59, charles.baylis@linaro.org wrote:
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -28208,6 +28208,35 @@ arm_expand_vec_perm (rtx target, rtx op0, rtx
>>> op1, rtx sel)
>>>      arm_expand_vec_perm_1 (target, op0, op1, sel);
>>>    }
>>>    +/* map lane ordering between architectural lane order, and GCC lane
>>> order,
>>> +   taking into account ABI.  See comment above output_move_neon for
>>> details.  */
>>> +static int
>>> +neon_endian_lane_map (machine_mode mode, int lane)
>>
>> s/map/Map/
>> New line between comment and function signature.
> Done.
>
>>> +{
>>> +  if (BYTES_BIG_ENDIAN)
>>> +  {
>>> +    int nelems = GET_MODE_NUNITS (mode);
>>> +    /* Reverse lane order.  */
>>> +    lane = (nelems - 1 - lane);
>>> +    /* Reverse D register order, to match ABI.  */
>>> +    if (GET_MODE_SIZE (mode) == 16)
>>> +      lane = lane ^ (nelems / 2);
>>> +  }
>>> +  return lane;
>>> +}
>>> +
>>> +/* some permutations index into pairs of vectors, this is a helper
>>> function
>>> +   to map indexes into those pairs of vectors.  */
>>> +static int
>>> +neon_pair_endian_lane_map (machine_mode mode, int lane)
>>
>> Similarly, s/some/Some/ and new line after comment.
> Done.
>
>>> +{
>>> +  int nelem = GET_MODE_NUNITS (mode);
>>> +  if (BYTES_BIG_ENDIAN)
>>> +    lane =
>>> +      neon_endian_lane_map (mode, lane & (nelem - 1)) + (lane & nelem);
>>> +  return lane;
>>> +}
>>> +
>>>    /* Generate or test for an insn that supports a constant permutation.
>>> */
>>>      /* Recognize patterns for the VUZP insns.  */
>>> @@ -28218,14 +28247,22 @@ arm_evpc_neon_vuzp (struct expand_vec_perm_d *d)
>>>      unsigned int i, odd, mask, nelt = d->nelt;
>>>      rtx out0, out1, in0, in1;
>>>      rtx (*gen)(rtx, rtx, rtx, rtx);
>>> +  int first_elem;
>>> +  int swap;
>>>
>> Just make this a bool.
> As discussed on IRC, this variable does contain an integer. I have
> renamed it as swap_nelt, and changed the test on it below.

This is ok.
Thanks,
Kyrill

> [snip]
>
>>>    @@ -28258,10 +28296,9 @@ arm_evpc_neon_vuzp (struct expand_vec_perm_d
>>> *d)
>>>        in0 = d->op0;
>>>      in1 = d->op1;
>>> -  if (BYTES_BIG_ENDIAN)
>>> +  if (swap)
>>>        {
>>>          std::swap (in0, in1);
>>> -      odd = !odd;
>>>        }
>> remove the braces around the std::swap
> Done. Also changed if (swap) to if (swap_nelt != 0)
>
> [snip]
>
>>> @@ -0,0 +1,24 @@
>>> +/* { dg-options "-O2 -ftree-vectorize -fno-vect-cost-model" } */
>>> +
>>> +#define SIZE 128
>>> +unsigned short _Alignas (16) in[SIZE];
>>> +
>>> +extern void abort (void);
>>> +
>>> +__attribute__ ((noinline)) int
>>> +test (unsigned short sum, unsigned short *in, int x)
>>> +{
>>> +  for (int j = 0; j < SIZE; j += 8)
>>> +    sum += in[j] * x;
>>> +  return sum;
>>> +}
>>> +
>>> +int
>>> +main ()
>>> +{
>>> +  for (int i = 0; i < SIZE; i++)
>>> +    in[i] = i;
>>> +  if (test (0, in, 1) != 960)
>>> +    abort ();
>>
>> AFAIK tests here usually prefer __builtin_abort ();
>> That way you don't have to declare the abort prototype in the beginning.
> Done.
>
> Updated patch attached