On Wed, Mar 23, 2016 at 03:06:22PM +0300, Kirill Yukhin wrote:
> On 23 Mar 12:55, Jakub Jelinek wrote:
> > On Wed, Mar 23, 2016 at 02:47:54PM +0300, Kirill Yukhin wrote:
> > > > This code is only executed if standard_sse_constant_p returns 2, which
> > > > is for 16-byte vectors and all ones for TARGET_SSE2, and for
> > > > 32-byte vectors for TARGET_AVX2.
> > > > Thus, IMNSHO the patch is wrong, even for plain -mavx -mno-avx2
> > > > we want to emit vpcmpeqd %xmm?, %xmm?, %xmm?, so that we don't mix
> > > > VEX with non-VEX encoded insns.
> > > You're right. I've updated the patch to check mode size as well.
> >
> > That is also wrong, you will then emit vpcmpeqd even for -msse2 -mno-avx.
> >
> > There is really nothing wrong in what is on the trunk, no reason to patch
> > anything. What the trunk does is, if you have AVX, you emit VEX encoded
> > insn, if you don't have AVX, you emit normally encoded insn.
> > And whether there is support to get cheap all ones is the business of
> > standard_sse_constant_p function, which DTRT.
> Probably, yes. But this just look pretty strange at first glance.
> Something like this looks much more straightforward to me:
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 963cc3d..4aff647 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -10859,7 +10859,7 @@ standard_sse_constant_opcode (rtx_insn *insn, rtx x)
> || get_attr_mode (insn) == MODE_V8DF
> || get_attr_mode (insn) == MODE_V16SF)
> return "vpternlogd\t{$0xFF, %g0, %g0, %g0|%g0, %g0, %g0, 0xFF}";
> - if (TARGET_AVX2 || get_attr_mode (insn) == 16)
> + if ((TARGET_AVX2 && get_attr_mode (insn) == 32) || TARGET_AVX)
> return "vpcmpeqd\t%0, %0, %0";
> else
> return "pcmpeqd\t%0, %0";
>
> But yes, if standard_sse_constant_p () is used properly there's no need for
> the change.
Even if used improperly. standard_sse_constant_opcode starts by calling
standard_sse_constant_p. So there is no point duplicating anything from it,
it has been already checked.
And
((TARGET_AVX2 && get_attr_mode (insn) == 32) || TARGET_AVX)
is just more fancy and expensive equivalent to
(TARGET_AVX)
(hint, if (TARGET_AVX2), then we know TARGET_AVX is non-zero too).
There is just nothing to guard, you don't want to ever emit pcmpeqd if
TARGET_AVX, simply because you don't want to mix VEX with old encoding,
as there is some penalty for that on some of the older CPUs.
Jakub
