Ping? thanks, Cong
On Tue, Jul 8, 2014 at 8:23 PM, Xinliang David Li <davi...@google.com> wrote: > Cong, can you ping this patch again? There does not seem to be > pending comments left. > > David > > On Tue, Dec 17, 2013 at 10:05 AM, Cong Hou <co...@google.com> wrote: >> Ping? >> >> >> thanks, >> Cong >> >> >> On Mon, Dec 2, 2013 at 5:02 PM, Cong Hou <co...@google.com> wrote: >>> Any comment on this patch? >>> >>> >>> thanks, >>> Cong >>> >>> >>> On Fri, Nov 22, 2013 at 11:40 AM, Cong Hou <co...@google.com> wrote: >>>> On Fri, Nov 22, 2013 at 3:57 AM, Marc Glisse <marc.gli...@inria.fr> wrote: >>>>> On Thu, 21 Nov 2013, Cong Hou wrote: >>>>> >>>>>> On Thu, Nov 21, 2013 at 4:39 PM, Marc Glisse <marc.gli...@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. >>>> >>>> >>>> I agree. Then I will wait until your patch is merged to the trunk, >>>> otherwise my patch could not pass the test. >>>> >>>> >>>>> >>>>> >>>>>> 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. >>>>> >>>> >>>> >>>> I also only found the one by Microsoft.. If the second argument is >>>> ignored, we could just remove it, as long as there is no "standard" >>>> that requires two arguments. Hopefully it won't break current projects >>>> using _mm_frcz_sd. >>>> >>>> Thank you for your comments! >>>> >>>> >>>> Cong >>>> >>>> >>>>> -- >>>>> Marc Glisse