Re: [Patch ARM] Fix PR71778

[James Greenhalgh <james.greenhalgh@arm.com>]

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

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