Re: [Patch ARM] Fix PR71778

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

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,
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.
>