On Wed, Oct 1, 2014 at 4:12 PM, Jakub Jelinek <[email protected]> wrote:
> On Wed, Oct 01, 2014 at 03:09:59PM +0200, Uros Bizjak wrote:
>> > 2014-10-01 Jakub Jelinek <[email protected]>
>> >
>> > * 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.
