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
