[Patch ARM] Fix PR71778

[Patch ARM] Fix PR71778

[James Greenhalgh]

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


Hi,

PR71778 is an ICE when you pass a non-constant argument to an intrinsic
which requires a constant.

This ICE was introduced after we rewrote some of the builtin handling for
Neon intrinsics, the issue is that after throwing an error in
arm_expand_builtin_args, we return const0_rtx to indicate the expand
has failed

      if (!(*insn_data[icode].operand[opno].predicate)
	  (op[argc], mode[argc]))
	{
	  error ("%Kargument %d must be a constant immediate",
		 exp, argc + 1);
	  return const0_rtx;
	}

At this point we're safely in to invalid code, but the mid-end continues
trying to resolve the assignment, with const0_rtx on the right-hand
side. That gets in to trouble in movv2di, which sees the constant and
tries to expand through neon_make_constant, which doesn't expect to see
a const0_rtx in the assignment (it wants a vector), and we hit a
gcc_unreachable () and take the ICE.

There are a few moving parts in the back end, so it isn't clear to me that
the fix I've come up with is 100% in the right place. AArch64 doesn't
bother with a similar construct, expanding straight to a mov with whatever
you've given it, so I don't see a right place over there.

The change is defensible, but I don't really know the ARM back end.

Bootstrapped on arm-none-linux-gnueabihf.

OK?

Thanks,
James

---
gcc/

2017-06-12  James Greenhalgh  <james.greenhalgh@arm.com>

	PR target/71778
	* config/arm/arm.c (neon_make_constant): Return const0_rtx for
	const0_rtx input.

gcc/testsuite/

2017-06-12  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-Patch-ARM-Fix-PR71778.patch --]
[-- Type: text/x-patch; name="0001-Patch-ARM-Fix-PR71778.patch", Size: 1533 bytes --]

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 ();
 
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);
+}
+

Re: [Patch ARM] Fix PR71778

[Kyrill Tkachov]

Hi James,

On 12/06/17 14:57, James Greenhalgh wrote:
> Hi,
>
> PR71778 is an ICE when you pass a non-constant argument to an intrinsic
> which requires a constant.
>
> This ICE was introduced after we rewrote some of the builtin handling for
> Neon intrinsics, the issue is that after throwing an error in
> arm_expand_builtin_args, we return const0_rtx to indicate the expand
> has failed
>
>        if (!(*insn_data[icode].operand[opno].predicate)
> 	  (op[argc], mode[argc]))
> 	{
> 	  error ("%Kargument %d must be a constant immediate",
> 		 exp, argc + 1);
> 	  return const0_rtx;
> 	}
>
> At this point we're safely in to invalid code, but the mid-end continues
> trying to resolve the assignment, with const0_rtx on the right-hand
> side. That gets in to trouble in movv2di, which sees the constant and
> tries to expand through neon_make_constant, which doesn't expect to see
> a const0_rtx in the assignment (it wants a vector), and we hit a
> gcc_unreachable () and take the ICE.

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

> There are a few moving parts in the back end, so it isn't clear to me that
> the fix I've come up with is 100% in the right place. AArch64 doesn't
> bother with a similar construct, expanding straight to a mov with whatever
> you've given it, so I don't see a right place over there.
>
> The change is defensible, but I don't really know the ARM back end.
>
> Bootstrapped on arm-none-linux-gnueabihf.
>
> OK?
>
> Thanks,
> James
>
> ---
> gcc/
>
> 2017-06-12  James Greenhalgh  <james.greenhalgh@arm.com>
>
> 	PR target/71778
> 	* config/arm/arm.c (neon_make_constant): Return const0_rtx for
> 	const0_rtx input.
>
> gcc/testsuite/
>
> 2017-06-12  James Greenhalgh  <james.greenhalgh@arm.com>
>
> 	PR target/71778
> 	* gcc.target/arm/pr71778.c: New.
>

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?

Thanks,
Kyrill

Re: [Patch ARM] Fix PR71778

[James Greenhalgh]

[-- 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);
+}
+

Re: [Patch ARM] Fix PR71778

[Kyrill Tkachov]

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

Re: [Patch ARM] Fix PR71778

[James Greenhalgh]

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

Re: [Patch ARM] Fix PR71778

[Kyrill Tkachov]

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