* [Patch ARM] Fix PR71778
@ 2017-06-12 13:58 James Greenhalgh
2017-06-14 10:21 ` Kyrill Tkachov
0 siblings, 1 reply; 6+ messages in thread
From: James Greenhalgh @ 2017-06-12 13:58 UTC (permalink / raw)
To: gcc-patches; +Cc: nd, ramana.radhakrishnan, kyrtka01, richard.earnshaw, nickc
[-- 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);
+}
+
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch ARM] Fix PR71778
2017-06-12 13:58 [Patch ARM] Fix PR71778 James Greenhalgh
@ 2017-06-14 10:21 ` Kyrill Tkachov
2017-06-16 9:07 ` James Greenhalgh
0 siblings, 1 reply; 6+ messages in thread
From: Kyrill Tkachov @ 2017-06-14 10:21 UTC (permalink / raw)
To: James Greenhalgh, gcc-patches
Cc: nd, ramana.radhakrishnan, kyrtka01, richard.earnshaw, nickc
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch ARM] Fix PR71778
2017-06-14 10:21 ` Kyrill Tkachov
@ 2017-06-16 9:07 ` James Greenhalgh
2017-06-16 10:07 ` Kyrill Tkachov
0 siblings, 1 reply; 6+ messages in thread
From: James Greenhalgh @ 2017-06-16 9:07 UTC (permalink / raw)
To: gcc-patches
Cc: nd, kyrylo.tkachov, richard.earnshaw, ramana.radhakrishnan, nickc
[-- 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);
+}
+
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch ARM] Fix PR71778
2017-06-16 9:07 ` James Greenhalgh
@ 2017-06-16 10:07 ` Kyrill Tkachov
2017-06-19 16:17 ` James Greenhalgh
0 siblings, 1 reply; 6+ messages in thread
From: Kyrill Tkachov @ 2017-06-16 10:07 UTC (permalink / raw)
To: James Greenhalgh, gcc-patches
Cc: nd, richard.earnshaw, ramana.radhakrishnan, nickc
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch ARM] Fix PR71778
2017-06-16 10:07 ` Kyrill Tkachov
@ 2017-06-19 16:17 ` James Greenhalgh
2017-06-19 16:25 ` Kyrill Tkachov
0 siblings, 1 reply; 6+ messages in thread
From: James Greenhalgh @ 2017-06-19 16:17 UTC (permalink / raw)
To: Kyrill Tkachov
Cc: gcc-patches, nd, richard.earnshaw, ramana.radhakrishnan, nickc
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.
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch ARM] Fix PR71778
2017-06-19 16:17 ` James Greenhalgh
@ 2017-06-19 16:25 ` Kyrill Tkachov
0 siblings, 0 replies; 6+ messages in thread
From: Kyrill Tkachov @ 2017-06-19 16:25 UTC (permalink / raw)
To: James Greenhalgh
Cc: gcc-patches, nd, richard.earnshaw, ramana.radhakrishnan, nickc
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.
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-19 16:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-12 13:58 [Patch ARM] Fix PR71778 James Greenhalgh
2017-06-14 10:21 ` Kyrill Tkachov
2017-06-16 9:07 ` James Greenhalgh
2017-06-16 10:07 ` Kyrill Tkachov
2017-06-19 16:17 ` James Greenhalgh
2017-06-19 16:25 ` Kyrill Tkachov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).