public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] rs6000: ICE when using an MMA type as a function param
@ 2020-08-10  3:03 Peter Bergner
  2020-08-10  3:07 ` Peter Bergner
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Peter Bergner @ 2020-08-10  3:03 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt

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.


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rs6000: ICE when using an MMA type as a function param
  2020-08-10  3:03 [PATCH] rs6000: ICE when using an MMA type as a function param Peter Bergner
@ 2020-08-10  3:07 ` Peter Bergner
  2020-08-10 18:30 ` Peter Bergner
  2020-08-12  2:00 ` Segher Boessenkool
  2 siblings, 0 replies; 14+ messages in thread
From: Peter Bergner @ 2020-08-10  3:07 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rs6000: ICE when using an MMA type as a function param
  2020-08-10  3:03 [PATCH] rs6000: ICE when using an MMA type as a function param Peter Bergner
  2020-08-10  3:07 ` Peter Bergner
@ 2020-08-10 18:30 ` Peter Bergner
  2020-08-12  2:00 ` Segher Boessenkool
  2 siblings, 0 replies; 14+ messages in thread
From: Peter Bergner @ 2020-08-10 18:30 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rs6000: ICE when using an MMA type as a function param
  2020-08-10  3:03 [PATCH] rs6000: ICE when using an MMA type as a function param Peter Bergner
  2020-08-10  3:07 ` Peter Bergner
  2020-08-10 18:30 ` Peter Bergner
@ 2020-08-12  2:00 ` Segher Boessenkool
  2020-08-12  2:07   ` Peter Bergner
  2020-08-12 19:24   ` Peter Bergner
  2 siblings, 2 replies; 14+ messages in thread
From: Segher Boessenkool @ 2020-08-12  2:00 UTC (permalink / raw)
  To: Peter Bergner; +Cc: GCC Patches, Bill Schmidt

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rs6000: ICE when using an MMA type as a function param
  2020-08-12  2:00 ` Segher Boessenkool
@ 2020-08-12  2:07   ` Peter Bergner
  2020-08-12  2:46     ` Segher Boessenkool
  2020-08-12 19:24   ` Peter Bergner
  1 sibling, 1 reply; 14+ messages in thread
From: Peter Bergner @ 2020-08-12  2:07 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rs6000: ICE when using an MMA type as a function param
  2020-08-12  2:07   ` Peter Bergner
@ 2020-08-12  2:46     ` Segher Boessenkool
  0 siblings, 0 replies; 14+ messages in thread
From: Segher Boessenkool @ 2020-08-12  2:46 UTC (permalink / raw)
  To: Peter Bergner; +Cc: GCC Patches, Bill Schmidt

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rs6000: ICE when using an MMA type as a function param
  2020-08-12  2:00 ` Segher Boessenkool
  2020-08-12  2:07   ` Peter Bergner
@ 2020-08-12 19:24   ` Peter Bergner
  2020-08-12 20:32     ` [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506] Peter Bergner
  2020-08-13  0:49     ` [PATCH] rs6000: ICE when using an MMA type as a function param Segher Boessenkool
  1 sibling, 2 replies; 14+ messages in thread
From: Peter Bergner @ 2020-08-12 19:24 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506]
  2020-08-12 19:24   ` Peter Bergner
@ 2020-08-12 20:32     ` Peter Bergner
  2020-08-13  1:00       ` Segher Boessenkool
  2020-08-13  0:49     ` [PATCH] rs6000: ICE when using an MMA type as a function param Segher Boessenkool
  1 sibling, 1 reply; 14+ messages in thread
From: Peter Bergner @ 2020-08-12 20:32 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Bill Schmidt, GCC Patches

rs6000: ICE when using an MMA type as a function param or return value [PR96506]

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 functionally identical to the previous patch.
The differences are that I've added more comments around the use of the
static local variable and I added prototypes for the test case's extern
bar* functions which allowed me to remove the now unneeded -w option.

Ok for trunk now?  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.

diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
index 189497efb45..869e4973a16 100644
--- a/gcc/config/rs6000/rs6000-call.c
+++ b/gcc/config/rs6000/rs6000-call.c
@@ -6444,8 +6444,26 @@ 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)
 {
+  /* Warning: this is a static local variable and not always NULL!  */
+  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))
+    {
+      /* Record we have now handled function CFUN, so the next time we
+	 are called, we do not re-report the same error.  */
+      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 +7414,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..b1b40c5a5c8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr96506.c
@@ -0,0 +1,66 @@
+/* PR target/96506 */
+/* { dg-do compile } */
+/* { dg-require-effective-target power10_ok } */
+/* { dg-options "-mdejagnu-cpu=power10 -O2" } */
+
+extern void bar0();
+extern void bar1();
+extern void bar2();
+extern void bar3();
+
+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;
+}


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rs6000: ICE when using an MMA type as a function param
  2020-08-12 19:24   ` Peter Bergner
  2020-08-12 20:32     ` [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506] Peter Bergner
@ 2020-08-13  0:49     ` Segher Boessenkool
  1 sibling, 0 replies; 14+ messages in thread
From: Segher Boessenkool @ 2020-08-13  0:49 UTC (permalink / raw)
  To: Peter Bergner; +Cc: GCC Patches, Bill Schmidt

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506]
  2020-08-12 20:32     ` [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506] Peter Bergner
@ 2020-08-13  1:00       ` Segher Boessenkool
  2020-08-13  1:59         ` Peter Bergner
  0 siblings, 1 reply; 14+ messages in thread
From: Segher Boessenkool @ 2020-08-13  1:00 UTC (permalink / raw)
  To: Peter Bergner; +Cc: Bill Schmidt, GCC Patches

Hi!

On Wed, Aug 12, 2020 at 03:32:18PM -0500, Peter Bergner wrote:
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -6444,8 +6444,26 @@ 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)
>  {
> +  /* Warning: this is a static local variable and not always NULL!  */
> +  static struct function *fn = NULL;

It may be just me that always misses "static" on locals, heh.  But
please comment what this is *for*: to warn only once per function.  You
could choose a better variable name to say that, too.

"struct function" is GTY, will this work this way, btw?

So I am worried about that; other than that, this is just fine (if you
tune the comment a bit).

Thanks,


Segher

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506]
  2020-08-13  1:00       ` Segher Boessenkool
@ 2020-08-13  1:59         ` Peter Bergner
  2020-08-13 18:58           ` Peter Bergner
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Bergner @ 2020-08-13  1:59 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Bill Schmidt, GCC Patches

On 8/12/20 8:00 PM, Segher Boessenkool wrote:
> On Wed, Aug 12, 2020 at 03:32:18PM -0500, Peter Bergner wrote:
>> --- a/gcc/config/rs6000/rs6000-call.c
>> +++ b/gcc/config/rs6000/rs6000-call.c
>> @@ -6444,8 +6444,26 @@ 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)
>>  {
>> +  /* Warning: this is a static local variable and not always NULL!  */
>> +  static struct function *fn = NULL;
> 
> It may be just me that always misses "static" on locals, heh.  But
> please comment what this is *for*: to warn only once per function.  You
> could choose a better variable name to say that, too.

Ok, how about this comment then?

@@ -6444,8 +6444,30 @@ 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)
 {
+  /* Warning: this is a static local variable and not always NULL!
+     This function is called multiple times for the same function
+     and return value.  PREV_FUNC is used to keep track of the
+     first time we encounter a function's return value in order
+     to not report an error with that return value multiple times.  */
+  static struct function *prev_func = 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
+      && prev_func != cfun
+      && (mode == POImode || mode == PXImode))
+    {
+      /* Record we have now handled function CFUN, so the next time we
+        are called, we do not re-report the same error.  */
+      prev_func = 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;

Peter

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506]
  2020-08-13  1:59         ` Peter Bergner
@ 2020-08-13 18:58           ` Peter Bergner
  2020-08-13 21:27             ` Segher Boessenkool
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Bergner @ 2020-08-13 18:58 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Bill Schmidt, GCC Patches

On 8/12/20 8:59 PM, Peter Bergner wrote:
> On 8/12/20 8:00 PM, Segher Boessenkool wrote:
>> On Wed, Aug 12, 2020 at 03:32:18PM -0500, Peter Bergner wrote:
> Ok, how about this comment then?
> 
> @@ -6444,8 +6444,30 @@ 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)
>  {
> +  /* Warning: this is a static local variable and not always NULL!
> +     This function is called multiple times for the same function
> +     and return value.  PREV_FUNC is used to keep track of the
> +     first time we encounter a function's return value in order
> +     to not report an error with that return value multiple times.  */
> +  static struct function *prev_func = NULL;

Approved offline, so I pushed this to trunk.  Thanks!

Are we ok to backport this to GCC 10?  If you don't want this
trickery in GCC 10, we could just backport the param handling
which doesn't use the trickery and leave the return value
unhandled.

Peter




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506]
  2020-08-13 18:58           ` Peter Bergner
@ 2020-08-13 21:27             ` Segher Boessenkool
  2020-08-19  2:58               ` Peter Bergner
  0 siblings, 1 reply; 14+ messages in thread
From: Segher Boessenkool @ 2020-08-13 21:27 UTC (permalink / raw)
  To: Peter Bergner; +Cc: Bill Schmidt, GCC Patches

On Thu, Aug 13, 2020 at 01:58:31PM -0500, Peter Bergner wrote:
> On 8/12/20 8:59 PM, Peter Bergner wrote:
> > On 8/12/20 8:00 PM, Segher Boessenkool wrote:
> >> On Wed, Aug 12, 2020 at 03:32:18PM -0500, Peter Bergner wrote:
> > Ok, how about this comment then?
> > 
> > @@ -6444,8 +6444,30 @@ 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)
> >  {
> > +  /* Warning: this is a static local variable and not always NULL!
> > +     This function is called multiple times for the same function
> > +     and return value.  PREV_FUNC is used to keep track of the
> > +     first time we encounter a function's return value in order
> > +     to not report an error with that return value multiple times.  */
> > +  static struct function *prev_func = NULL;
> 
> Approved offline, so I pushed this to trunk.  Thanks!
> 
> Are we ok to backport this to GCC 10?  If you don't want this
> trickery in GCC 10, we could just backport the param handling
> which doesn't use the trickery and leave the return value
> unhandled.

It's okay for backporting as well.  It's all kind of wrong, but it will
in practice just work anyway:

1) struct function is GTY, and you don't mark the static variable here
as root, so the thing it points into might have gone away; but we really
only use the pointer here, we don't deref anything explicitly, so the
generated program won't either (hopefully, etc.)

2) Similarly, some other struct function for another function may (in
theory) be allocated at the same address when next we are called; that
other function will then never get the warning.

We'd need to store a flag in the struct function (or similarly) itself,
to make things kosher.  How do similar warnings elsewhere handle this?


Anyway, okay for trunk and backports.  Thanks!


Segher

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506]
  2020-08-13 21:27             ` Segher Boessenkool
@ 2020-08-19  2:58               ` Peter Bergner
  0 siblings, 0 replies; 14+ messages in thread
From: Peter Bergner @ 2020-08-19  2:58 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Bill Schmidt, GCC Patches

On 8/13/20 4:27 PM, Segher Boessenkool wrote:
> Anyway, okay for trunk and backports.  Thanks!

Ok, I committed this to trunk and waited a few days and then
pushed this to GCC 10 release branch too.  Thanks!

Peter


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-08-19  2:58 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-10  3:03 [PATCH] rs6000: ICE when using an MMA type as a function param Peter Bergner
2020-08-10  3:07 ` Peter Bergner
2020-08-10 18:30 ` Peter Bergner
2020-08-12  2:00 ` Segher Boessenkool
2020-08-12  2:07   ` Peter Bergner
2020-08-12  2:46     ` Segher Boessenkool
2020-08-12 19:24   ` Peter Bergner
2020-08-12 20:32     ` [PATCH v2] rs6000: ICE when using an MMA type as a function param or return value [PR96506] Peter Bergner
2020-08-13  1:00       ` Segher Boessenkool
2020-08-13  1:59         ` Peter Bergner
2020-08-13 18:58           ` Peter Bergner
2020-08-13 21:27             ` Segher Boessenkool
2020-08-19  2:58               ` Peter Bergner
2020-08-13  0:49     ` [PATCH] rs6000: ICE when using an MMA type as a function param Segher Boessenkool

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