Re: [Patch ARM] Fix PR71778

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

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]


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.

Bootstrapped on arm-none-linux-gnueabihf. OK?

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Re-Patch-ARM-Fix-PR71778.patch --]
[-- Type: text/x-patch; name="0001-Re-Patch-ARM-Fix-PR71778.patch", Size: 1646 bytes --]

diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c
index a0569ed..8ecf581 100644
--- a/gcc/config/arm/arm-builtins.c
+++ b/gcc/config/arm/arm-builtins.c
@@ -2245,7 +2245,12 @@ constant_arg:
 		{
 		  error ("%Kargument %d must be a constant immediate",
 			 exp, argc + 1);
-		  return const0_rtx;
+		  /* We have failed to expand the pattern, and are safely
+		     in to invalid code.  But the mid-end will still try to
+		     build an assignment for this node while it expands,
+		     before stopping for the error, just pass it back
+		     TARGET to ensure a valid assignment.  */
+		  return target;
 		}
 	      break;
 
diff --git a/gcc/testsuite/gcc.target/arm/pr71778.c b/gcc/testsuite/gcc.target/arm/pr71778.c
new file mode 100644
index 0000000..d5b0d04
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr71778.c
@@ -0,0 +1,24 @@
+/* { dg-do compile }  */
+/* { dg-require-effective-target arm_neon_ok } */
+/* { dg-options "-O2" } */
+/* { dg-add-options arm_neon } */
+
+typedef __simd128_int32_t int32x4_t;
+
+__extension__ extern __inline int32x4_t
+__attribute__  ((__always_inline__, __gnu_inline__, __artificial__))
+vshrq_n_s32 (int32x4_t __a, const int __b)
+{
+  /* Errors for arm_neon.h intrinsics using constants end up on the line
+     in arm_neon.h rather than the source file line.  That means we
+     need to put the dg-error up here, rather than on line 22 where we'd
+     like it.  */
+  return (int32x4_t)__builtin_neon_vshrs_nv4si (__a, __b); /* { dg-error "argument 2 must be a constant immediate" } */
+}
+
+int32x4_t
+shift (int32x4_t a, int b)
+{
+  return vshrq_n_s32 (a, b);
+}
+