Message ID | 397d63db-9190-e875-44a9-b165964a5199@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | rs6000: ICE when using an MMA type as a function param | expand |
On 8/9/20 10:03 PM, Peter Bergner wrote: > gcc/ > PR target/96506 > * config/rs6000/rs6000-call.c (rs6000_promote_function_mode): > (rs6000_function_arg): Oops, missed the ChangeLog entries: gcc/ PR target/96506 * config/rs6000/rs6000-call.c (rs6000_promote_function_mode): Disallow MMA types as return values. (rs6000_function_arg): Disallow MMA types as function arguments. Peter
On 8/9/20 10:03 PM, Peter Bergner wrote: > PR96506 shows a problem where we ICE on illegal usage, namely using MMA > types for function arguments and return values. The solution is to flag > these illegal usages as errors early, before we ICE. > > The patch below is currently bootstrapping and regtesting. Ok for trunk > once that comes back clean? Ok for GCC 10 after some bake in? > > Peter > > > gcc/ > PR target/96506 > * config/rs6000/rs6000-call.c (rs6000_promote_function_mode): Disallow > MMA types as return values. > (rs6000_function_arg): Disallow MMA types as function arguments. > > gcc/testsuite/ > PR target/96506 > * gcc.target/powerpc/pr96506.c: New test. FYI, testing came back clean with no regressions. Peter
Hi! Not just params, but return values as well. "Error on MMA types in function prototype"? On Sun, Aug 09, 2020 at 10:03:35PM -0500, Peter Bergner wrote: > --- a/gcc/config/rs6000/rs6000-call.c > +++ b/gcc/config/rs6000/rs6000-call.c > @@ -6444,8 +6444,23 @@ machine_mode > rs6000_promote_function_mode (const_tree type ATTRIBUTE_UNUSED, > machine_mode mode, > int *punsignedp ATTRIBUTE_UNUSED, > - const_tree, int) > + const_tree, int for_return) > { > + static struct function *fn = NULL; > + > + /* We do not allow MMA types being used as return values. Only report > + the invalid return value usage the first time we encounter it. */ > + if (for_return > + && fn != cfun > + && (mode == POImode || mode == PXImode)) "fn" is always zero here. > + { > + fn = cfun; And what you set here is unused. So just remove fn? > + if (TYPE_CANONICAL (type) != NULL_TREE) != NULL_TREE != false != 0 (sorry sorry) > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr96506.c > @@ -0,0 +1,61 @@ > +/* PR target/96506 */ > +/* { dg-do compile } */ > +/* { dg-require-effective-target power10_ok } */ > +/* { dg-options "-mdejagnu-cpu=power10 -O2 -w" } */ Do you need -w or could a less heavy hammer work as well? Okay for trunk (and backports after some simmering) with those things looked at. Thanks! Segher
On 8/11/20 9:00 PM, Segher Boessenkool wrote: > Not just params, but return values as well. "Error on MMA types in > function prototype"? Yes, it started out as a function param issue and then while working on this, I decided I better look at what happens when they're used as return values. I'll update the commit message to include return values. >> + static struct function *fn = NULL; >> + >> + /* We do not allow MMA types being used as return values. Only report >> + the invalid return value usage the first time we encounter it. */ >> + if (for_return >> + && fn != cfun >> + && (mode == POImode || mode == PXImode)) > > "fn" is always zero here. > >> + { >> + fn = cfun; > > And what you set here is unused. It's a static local variable, so how is it always zero and unused? >> +/* { dg-options "-mdejagnu-cpu=power10 -O2 -w" } */ > > Do you need -w or could a less heavy hammer work as well? I could probably declare bar0(), bar1(), bar2() and bar3() and those might go away? I didn't for some reason, but that may have been for some earlier iteration of the test case. I'll have a look at removing that. Peter
On Tue, Aug 11, 2020 at 09:07:40PM -0500, Peter Bergner wrote: > >> + static struct function *fn = NULL; > >> + > >> + /* We do not allow MMA types being used as return values. Only report > >> + the invalid return value usage the first time we encounter it. */ > >> + if (for_return > >> + && fn != cfun > >> + && (mode == POImode || mode == PXImode)) > > > > "fn" is always zero here. > > > >> + { > >> + fn = cfun; > > > > And what you set here is unused. > > It's a static local variable, so how is it always zero and unused? Oh, trickiness with it being called a second time. Ouch! This needs a H U G E comment then... Or better, get rid of that? Segher
On 8/11/20 9:00 PM, Segher Boessenkool wrote: > On Sun, Aug 09, 2020 at 10:03:35PM -0500, Peter Bergner wrote: >> +/* { dg-options "-mdejagnu-cpu=power10 -O2 -w" } */ > > Do you need -w or could a less heavy hammer work as well? So adding: extern void bar0(); etc. was enough to get rid of the warnings, so I'll add that and remove the use of -w. >> It's a static local variable, so how is it always zero and unused? > > Oh, trickiness with it being called a second time. Ouch! > > This needs a H U G E comment then... Or better, get rid of that? We cannot really get rid if it. With the code as is, we see for the following test: __vector_quad foo (__vector_quad *src) { return *src; } bug.i: In function ‘foo’: bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value 2 | foo (__vector_quad *src) | ^~~ versus without the fn != cfun condition: bug.i: In function ‘foo’: bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value 2 | foo (__vector_quad *src) | ^~~ bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value bug.i:2:1: error: invalid use of MMA type ‘__vector_quad’ as a function return value bug.i:4:10: error: invalid use of MMA type ‘__vector_quad’ as a function return value 4 | return *src; | ^~~~ I'll modify the test case and add a comment here and then resend the patch. Peter
On Wed, Aug 12, 2020 at 02:24:33PM -0500, Peter Bergner wrote: > On 8/11/20 9:00 PM, Segher Boessenkool wrote: > > On Sun, Aug 09, 2020 at 10:03:35PM -0500, Peter Bergner wrote: > >> +/* { dg-options "-mdejagnu-cpu=power10 -O2 -w" } */ > > > > Do you need -w or could a less heavy hammer work as well? > > So adding: > > extern void bar0(); etc. was enough to get rid of the warnings, so > I'll add that and remove the use of -w. Great. > >> It's a static local variable, so how is it always zero and unused? > > > > Oh, trickiness with it being called a second time. Ouch! > > > > This needs a H U G E comment then... Or better, get rid of that? > > We cannot really get rid if it. "Implement that some other way". Expecting the compiler to only call the param handling thing for the same function eight times in order, never interleaved with something else, is asking for trouble. But okay; just document the static :-) > I'll modify the test case and add a comment here and then resend the patch. Thanks! Segher
diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c index 189497efb45..e4ed88cd2f8 100644 --- a/gcc/config/rs6000/rs6000-call.c +++ b/gcc/config/rs6000/rs6000-call.c @@ -6444,8 +6444,23 @@ machine_mode rs6000_promote_function_mode (const_tree type ATTRIBUTE_UNUSED, machine_mode mode, int *punsignedp ATTRIBUTE_UNUSED, - const_tree, int) + const_tree, int for_return) { + static struct function *fn = NULL; + + /* We do not allow MMA types being used as return values. Only report + the invalid return value usage the first time we encounter it. */ + if (for_return + && fn != cfun + && (mode == POImode || mode == PXImode)) + { + fn = cfun; + if (TYPE_CANONICAL (type) != NULL_TREE) + type = TYPE_CANONICAL (type); + error ("invalid use of MMA type %qs as a function return value", + IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type)))); + } + PROMOTE_MODE (mode, *punsignedp, type); return mode; @@ -7396,6 +7411,16 @@ rs6000_function_arg (cumulative_args_t cum_v, const function_arg_info &arg) machine_mode elt_mode; int n_elts; + /* We do not allow MMA types being used as function arguments. */ + if (mode == POImode || mode == PXImode) + { + if (TYPE_CANONICAL (type) != NULL_TREE) + type = TYPE_CANONICAL (type); + error ("invalid use of MMA operand of type %qs as a function parameter", + IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type)))); + return NULL_RTX; + } + /* Return a marker to indicate whether CR1 needs to set or clear the bit that V.4 uses to say fp args were passed in registers. Assume that we don't need the marker for software floating point, diff --git a/gcc/testsuite/gcc.target/powerpc/pr96506.c b/gcc/testsuite/gcc.target/powerpc/pr96506.c new file mode 100644 index 00000000000..4ed31bc55fa --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr96506.c @@ -0,0 +1,61 @@ +/* PR target/96506 */ +/* { dg-do compile } */ +/* { dg-require-effective-target power10_ok } */ +/* { dg-options "-mdejagnu-cpu=power10 -O2 -w" } */ + +typedef __vector_pair vpair_t; +typedef __vector_quad vquad_t; + +/* Verify we flag errors on the following. */ + +void +foo0 (void) +{ + __vector_pair v; + bar0 (v); /* { dg-error "invalid use of MMA operand of type .__vector_pair. as a function parameter" } */ +} + +void +foo1 (void) +{ + vpair_t v; + bar1 (v); /* { dg-error "invalid use of MMA operand of type .__vector_pair. as a function parameter" } */ +} + +void +foo2 (void) +{ + __vector_quad v; + bar2 (v); /* { dg-error "invalid use of MMA operand of type .__vector_quad. as a function parameter" } */ +} + +void +foo3 (void) +{ + vquad_t v; + bar3 (v); /* { dg-error "invalid use of MMA operand of type .__vector_quad. as a function parameter" } */ +} + +__vector_pair +foo4 (__vector_pair *src) /* { dg-error "invalid use of MMA type .__vector_pair. as a function return value" } */ +{ + return *src; +} + +vpair_t +foo5 (vpair_t *src) /* { dg-error "invalid use of MMA type .__vector_pair. as a function return value" } */ +{ + return *src; +} + +__vector_quad +foo6 (__vector_quad *src) /* { dg-error "invalid use of MMA type .__vector_quad. as a function return value" } */ +{ + return *src; +} + +vquad_t +foo7 (vquad_t *src) /* { dg-error "invalid use of MMA type .__vector_quad. as a function return value" } */ +{ + return *src; +}