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
