Re: [Patch ARM] Fix PR71778

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

On 19/06/17 17:16, James Greenhalgh wrote:
> On Fri, Jun 16, 2017 at 11:07:41AM +0100, Kyrill Tkachov wrote:
>> On 16/06/17 10:07, James Greenhalgh wrote:
>>> On Wed, Jun 14, 2017 at 11:21:30AM +0100, Kyrill Tkachov wrote:
>>>
>>>    <...>
>>>
>>>> That movv2di expander is the one in vec-common.md that ends up calling
>>>> neon_make_constant. I wonder why const0_rtx passed its predicate check
>>>> (that would require a V2DImode vector of zeroes rather than a const0_rtx).
>>>> Perhaps the midend code at this point doesn't check the operand predicate.
>>>>
>>>> In the builtin expansion code that you quoted I wonder wonder if we could fail
>>>> more gracefully by returning CONST0_RTX (mode[argc]) to match the expected
>>>> mode of the operand (we've already emitted an error, so we shouldn't care
>>>> what RTL we emit as long as it doesn't cause an ICE).
>>>    <...>
>>>
>>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>>> index e503891..b8d59c6 100644
>>>> --- a/gcc/config/arm/arm.c
>>>> +++ b/gcc/config/arm/arm.c
>>>> @@ -12124,6 +12124,11 @@ neon_make_constant (rtx vals)
>>>>         if (n_const == n_elts)
>>>>   	const_vec = gen_rtx_CONST_VECTOR (mode, XVEC (vals, 0));
>>>>       }
>>>> +  else if (vals == const0_rtx)
>>>> +    /* Something invalid, perhaps from expanding an intrinsic
>>>> +       which requires a constant argument, where a variable argument
>>>> +       was passed.  */
>>>> +     return const0_rtx;
>>>>     else
>>>>       gcc_unreachable ();
>>>>
>>>> I'm not a fan of this as the function has a precondition that its argument is
>>>> a PARALLEL or a CONST_VECTOR and special-casing const0_rtx breaks that. I'd
>>>> rather we tried fixing this closer to the error source.  Can you try the
>>>> suggestion above instead please?
>>> Your suggestion doesn't quite work, but this is pretty close to it. Rather
>>> than try to guess at the correct mode for CONST0_RTX (we can't just use
>>> mode[argc] as that will get you the scalar mode), we can just return target
>>> directly. That will ensure we've given something valid back in the correct
>>> mode, even if it is not all that useful.
>> Yeah, that actually looks better.
>>
>>> Bootstrapped on arm-none-linux-gnueabihf. OK?
>> Ok.
> Thanks.
>
> The patch applies cleanly to gcc-7-branch and gcc-6-branch, both of which
> I've bootstrapped and tested on arm-none-linux-gnueabihf without issue.
>
> Is it OK for me to apply these backports and close out the PR (it is
> marked as a 6/7 regression).

Ok.
Thanks,
Kyrill

> Thanks,
> James
>
>
>>> ---
>>> gcc/
>>>
>>> 2017-06-15  James Greenhalgh  <james.greenhalgh@arm.com>
>>>
>>> 	PR target/71778
>>> 	* config/arm/arm-builtins.c (arm_expand_builtin_args): Return TARGET
>>> 	if given a non-constant argument for an intrinsic which requires a
>>> 	constant.
>>>
>>> gcc/testsuite/
>>>
>>> 2017-06-15  James Greenhalgh  <james.greenhalgh@arm.com>
>>>
>>> 	PR target/71778
>>> 	* gcc.target/arm/pr71778.c: New.
>>>