| Message ID | 1497604036-4653-1-git-send-email-james.greenhalgh@arm.com |
|---|---|
| State | New |
| Headers | show |
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. >
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. > > >
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. >>>
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); +} +