Re: [PATCH] Support addsub/subadd as non-isomorphic operations for SLP vectorizer.

[Marc Glisse <marc dot glisse at inria dot fr>]

On Thu, 21 Nov 2013, Cong Hou wrote:

> On Thu, Nov 21, 2013 at 4:39 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> > On Thu, 21 Nov 2013, Cong Hou wrote:
> >
> > > While I added the new define_insn_and_split for vec_merge, a bug is
> > > exposed: in config/i386/sse.md, [ define_expand "xop_vmfrcz<mode>2" ]
> > > only takes one input, but the corresponding builtin functions have two
> > > inputs, which are shown in i386.c:
> > >
> > >  { OPTION_MASK_ISA_XOP, CODE_FOR_xop_vmfrczv4sf2,
> > > "__builtin_ia32_vfrczss",     IX86_BUILTIN_VFRCZSS,     UNKNOWN,
> > > (int)MULTI_ARG_2_SF },
> > >  { OPTION_MASK_ISA_XOP, CODE_FOR_xop_vmfrczv2df2,
> > > "__builtin_ia32_vfrczsd",     IX86_BUILTIN_VFRCZSD,     UNKNOWN,
> > > (int)MULTI_ARG_2_DF },
> > >
> > > In consequence, the ix86_expand_multi_arg_builtin() function tries to
> > > check two args but based on the define_expand of xop_vmfrcz<mode>2,
> > > the content of insn_data[CODE_FOR_xop_vmfrczv4sf2].operand[2] may be
> > > incorrect (because it only needs one input).
> > >
> > > The patch below fixed this issue.
> > >
> > > Bootstrapped and tested on ax x86-64 machine. Note that this patch
> > > should be applied before the one I sent earlier (sorry for sending
> > > them in wrong order).
> >
> >
> > This is PR 56788. Your patch seems strange to me and I don't think it
> > fixes the real issue, but I'll let more knowledgeable people answer.
>
>
> Thank you for pointing out the bug report. This patch is not intended
> to fix PR56788.

IMHO, if PR56788 was fixed, you wouldn't have this issue, and if PR56788 
doesn't get fixed, I'll post a patch to remove _mm_frcz_sd and the 
associated builtin, which would solve your issue as well.

> For your function:
>
> #include <x86intrin.h>
> __m128d f(__m128d x, __m128d y){
>  return _mm_frcz_sd(x,y);
> }
>
> Note that the second parameter is ignored intentionally, but the
> prototype of this function contains two parameters. My fix is
> explicitly telling GCC that the optab xop_vmfrczv4sf3 should have
> three operands instead of two, to let it have the correct information
> in insn_data[CODE_FOR_xop_vmfrczv4sf3].operand[2] which is used to
> match the type of the second parameter in the builtin function in
> ix86_expand_multi_arg_builtin().

I disagree that this is intentional, it is a bug. AFAIK there is no AMD 
documentation that could be used as a reference for what _mm_frcz_sd is 
supposed to do. The only existing documentations are by Microsoft (which 
does *not* ignore the second argument) and by LLVM (which has a single 
argument). Whatever we chose for _mm_frcz_sd, the builtin should take a 
single argument, and if necessary we'll use 2 builtins to implement 
_mm_frcz_sd.

--
Marc Glisse