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

[Uros Bizjak <ubizjak at gmail dot com>]

On Wed, Oct 1, 2014 at 4:12 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Oct 01, 2014 at 03:09:59PM +0200, Uros Bizjak wrote:
>> > 2014-10-01  Jakub Jelinek  <jakub@redhat.com>
>> >
>> >         * config/i386/i386.c (expand_vec_perm_palignr): Handle
>> >         256-bit vectors for TARGET_AVX2.
>>
>> Please mention PR 62128 and include the testcase from the PR. Also,
>> please add a version of gcc.target/i386/pr52252-atom.c, compiled with
>> -mavx2 (perhaps named pr52252-avx2.c)
>
> This patch doesn't fix PR62128, and it is already tested (even without
> GCC_RUN_EXPENSIVE_TESTS=1) in the vshuf*.c torture tests (several of them).

Ah, OK then.

> If you want coverage not just for the default flags, I'd say we should
> say for -O2 only just add gcc.target/i386/{avx,avx2,avx512}-vshuf-*.c
> tests that would include ../../gcc.dg/torture/vshuf-*.c and be compiled/run
> with -mavx/-mavx2/-mavx512 options instead of the default ones.

No, the above should be good for now. The failure was triggered by the
target that defaults to -mavx2, and for avx512f we can run this suite
in the way you suggest.

> For PR62128, IMHO the right fix is attached.  Note, this is again covered
> in vshuf-*.c tests (test 22 in both vshuf-v32*.c and vshuf-v16*.c).
> With that attached patch, pr62128.c (aka test_22 in vshuf-v32qi.c), changes:
> -       vpshufb .LC0(%rip), %ymm0, %ymm1
> -       vpshufb .LC1(%rip), %ymm0, %ymm0
> -       vpermq  $78, %ymm1, %ymm1
> -       vpor    %ymm1, %ymm0, %ymm0
> +       vpermq  $78, %ymm0, %ymm1
> +       vpalignr        $1, %ymm0, %ymm1, %ymm0
>         ret
>
>> > --- gcc/config/i386/i386.c.jj   2014-10-01 14:24:16.483138899 +0200
>> > +++ gcc/config/i386/i386.c      2014-10-01 14:27:53.577222011 +0200
>> > @@ -43297,44 +43297,75 @@ expand_vec_perm_palignr (struct expand_v
>> >    rtx shift, target;
>> >    struct expand_vec_perm_d dcopy;
>> >
>> > -  /* Even with AVX, palignr only operates on 128-bit vectors.  */
>> > -  if (!TARGET_SSSE3 || GET_MODE_SIZE (d->vmode) != 16)
>> > +  /* Even with AVX, palignr only operates on 128-bit vectors,
>> > +     in AVX2 palignr operates on both 128-bit lanes.  */
>> > +  if ((!TARGET_SSSE3 || GET_MODE_SIZE (d->vmode) != 16)
>> > +      && (!TARGET_AVX2 || GET_MODE_SIZE (d->vmode) != 32))
>>
>> Please simplify the above condition ...
>
> How?  I don't see how that can be simplified, it can
> be transformed into
>   if (!((TARGET_SSSE3 && GET_MODE_SIZE (d->vmode) == 16)
>         || (TARGET_AVX2 && GET_MODE_SIZE (d->vmode) == 32)))
> but I don't find that simpler.

I was thinking of the above, but you are correct, the change doesn't
bring us anything.

So, the patch is OK as it was.

Thanks,
Uros.