Re: [PATCH 2/2, x86] Add palignr support for AVX2.

[Richard Henderson <rth at redhat dot com>]

On 04/29/2014 06:50 AM, Evgeny Stupachenko wrote:
> +  if (d->one_operand_p != true)
> +    return false;

This looks odd.  Better as !d->one_operand_p.

> +
> +  /* For an in order permutation with one operand like: {5 6 7 0 1 2 3 4}
> +     PALIGNR is better than PSHUFB.  Check for an order in permutation.  */

FWIW, "in order permutation" sounds like a contradiction in terms.
Better to describe this as a rotate.

> +  in_order_length = 0;
> +  in_order_length_max = 0;
> +  if (d->one_operand_p == true)

You've just tested one_operand above.

> +    for (i = 0; i < 2 * nelt; ++i)

Which means that 2*nelt is doing twice as much work as needed.

> +      {
> +       if ((d->perm[(i + 1) & (nelt - 1)] -
> +            d->perm[i & (nelt - 1)]) != 1)

Surely we can avoid re-reading the comparison value...

  next = (d->perm[0] + 1) & (nelt - 1);
  for (i = 1; i < nelt; ++i)
    {
      if (d->perm[i] != next)
        return false;
      next = (next + 1) & (nelt - 1);
    }

> +         {
> +           if (in_order_length > in_order_length_max)
> +               in_order_length_max = in_order_length;
> +             in_order_length = 0;
> +         }
> +       else
> +         in_order_length++;
> +      }
> +
> +  /* If not an ordered permutation then try something else.  */
> +  if (in_order_length_max != nelt - 1)
> +    return false;

I don't understand what this length and max stuff is trying to accomplish.

> +
> +  min = d->perm[0];
> +
> +  shift = GEN_INT (min * GET_MODE_BITSIZE (GET_MODE_INNER (d->vmode)));
> +  shift1 = GEN_INT ((min - nelt / 2) *
> +          GET_MODE_BITSIZE (GET_MODE_INNER (d->vmode)));
> +
> +  if (GET_MODE_SIZE (d->vmode) != 32)

Positive tests are almost always clearer: == 16.


r~