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
