Re: [PATCH, i386, AVX] Fix typo in i386.c/standard_sse_constant_opcode ().

[Kirill Yukhin <kirill dot yukhin at gmail dot com>]

>On 23 Mar 13:13, Jakub Jelinek wrote:
> 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.
Okay, got it, thanks!
> 
> 	Jakub