diff mbox series

rs6000: ICE when using an MMA type as a function param

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

Commit Message

Peter Bergner Aug. 10, 2020, 3:03 a.m. UTC 
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):
	(rs6000_function_arg):

gcc/testsuite/
	PR target/96506
	* gcc.target/powerpc/pr96506.c: New test.

Comments

Peter Bergner Aug. 10, 2020, 3:07 a.m. UTC | #1 
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
Peter Bergner Aug. 10, 2020, 6:30 p.m. UTC | #2 
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
Segher Boessenkool Aug. 12, 2020, 2 a.m. UTC | #3 
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
Peter Bergner Aug. 12, 2020, 2:07 a.m. UTC | #4 
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
Segher Boessenkool Aug. 12, 2020, 2:46 a.m. UTC | #5 
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
Peter Bergner Aug. 12, 2020, 7:24 p.m. UTC | #6 
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
Segher Boessenkool Aug. 13, 2020, 12:49 a.m. UTC | #7 
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 mbox series

Patch

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