[PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Peter Bergner]

PR87496 shows a bug where we ICE if we attempt to use -mabi=ieeelongdouble
and -mno-popcntd.  The IEEE128 support requires full ISA 2.06 (aka POWER7)
support, so we really should throw an error when using those options
together.  Ditto for -mabi=ieeelongdouble and -mno-vsx.  The patch below
does that.

Ok for mainline once bootstrap and regtesting are complete and clean?

Peter


gcc/
	PR target/87496
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disallow
	-mabi=ieeelongdouble without both -mpopcntd and -mvsx.

gcc/testsuite/
	PR target/87496
	* gcc.target/powerpc/pr87496.c: New test.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 266566)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -4291,16 +4291,22 @@ rs6000_option_override_internal (bool gl
   if (!global_options_set.x_rs6000_ieeequad)
     rs6000_ieeequad = TARGET_IEEEQUAD_DEFAULT;
 
-  else if (rs6000_ieeequad != TARGET_IEEEQUAD_DEFAULT && TARGET_LONG_DOUBLE_128)
+  else
     {
-      static bool warned_change_long_double;
-      if (!warned_change_long_double)
+      if (!TARGET_POPCNTD || !TARGET_VSX)
+	error ("%qs requires full ISA 2.06 support", "-mabi=ieeelongdouble");
+
+      if (rs6000_ieeequad != TARGET_IEEEQUAD_DEFAULT && TARGET_LONG_DOUBLE_128)
 	{
-	  warned_change_long_double = true;
-	  if (TARGET_IEEEQUAD)
-	    warning (OPT_Wpsabi, "Using IEEE extended precision long double");
-	  else
-	    warning (OPT_Wpsabi, "Using IBM extended precision long double");
+	  static bool warned_change_long_double;
+	  if (!warned_change_long_double)
+	    {
+	      warned_change_long_double = true;
+	      if (TARGET_IEEEQUAD)
+		warning (OPT_Wpsabi, "Using IEEE extended precision long double");
+	      else
+		warning (OPT_Wpsabi, "Using IBM extended precision long double");
+	    }
 	}
     }
 
Index: gcc/testsuite/gcc.target/powerpc/pr87496.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr87496.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr87496.c	(working copy)
@@ -0,0 +1,10 @@
+/* PR target/87496.c */
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
+/* { dg-require-effective-target longdouble128 } */
+/* { dg-options "-O2 -mcpu=power7 -mabi=ieeelongdouble -mno-popcntd -Wno-psabi" } */
+
+int i;
+
+/* { dg-error "'-mabi=ieeelongdouble' requires full ISA 2.06 support" "PR87496" { target *-*-* } 0 } */

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Peter Bergner]

On 11/28/18 3:27 PM, Peter Bergner wrote:
> Ok for mainline once bootstrap and regtesting are complete and clean?
> 
> Peter
> 
> 
> gcc/
> 	PR target/87496
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disallow
> 	-mabi=ieeelongdouble without both -mpopcntd and -mvsx.
> 
> gcc/testsuite/
> 	PR target/87496
> 	* gcc.target/powerpc/pr87496.c: New test.

FYI, bootstrap and regtesting completed with no regressions.

Peter

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Segher Boessenkool]

On Wed, Nov 28, 2018 at 03:27:19PM -0600, Peter Bergner wrote:
> PR87496 shows a bug where we ICE if we attempt to use -mabi=ieeelongdouble
> and -mno-popcntd.  The IEEE128 support requires full ISA 2.06 (aka POWER7)
> support, so we really should throw an error when using those options
> together.  Ditto for -mabi=ieeelongdouble and -mno-vsx.  The patch below
> does that.
> 
> Ok for mainline once bootstrap and regtesting are complete and clean?

Okay.  Eventually we shouldn't allow selecting popcntd independently from
-mcpu=, but that day isn't here yet.  So, okay for trunk, and backports
if wanted.  Thanks!


Segher


> 	PR target/87496
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disallow
> 	-mabi=ieeelongdouble without both -mpopcntd and -mvsx.
> 
> gcc/testsuite/
> 	PR target/87496
> 	* gcc.target/powerpc/pr87496.c: New test.

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Peter Bergner]

On 11/29/18 11:26 AM, Segher Boessenkool wrote:
> On Wed, Nov 28, 2018 at 03:27:19PM -0600, Peter Bergner wrote:
>> PR87496 shows a bug where we ICE if we attempt to use -mabi=ieeelongdouble
>> and -mno-popcntd.  The IEEE128 support requires full ISA 2.06 (aka POWER7)
>> support, so we really should throw an error when using those options
>> together.  Ditto for -mabi=ieeelongdouble and -mno-vsx.  The patch below
>> does that.
>>
>> Ok for mainline once bootstrap and regtesting are complete and clean?
> 
> Okay.  Eventually we shouldn't allow selecting popcntd independently from
> -mcpu=, but that day isn't here yet.  So, okay for trunk, and backports
> if wanted.  Thanks!

Ok, committed to mainline.  It looks like GCC8 needs the same patch.
I'll have to look closer at GCC7 on whether it needs it too, since the
code seems to be a little different.

Thanks

Peter

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Peter Bergner]

On 11/29/18 1:31 PM, Peter Bergner wrote:
> On 11/29/18 11:26 AM, Segher Boessenkool wrote:
>> On Wed, Nov 28, 2018 at 03:27:19PM -0600, Peter Bergner wrote:
>>> PR87496 shows a bug where we ICE if we attempt to use -mabi=ieeelongdouble
>>> and -mno-popcntd.  The IEEE128 support requires full ISA 2.06 (aka POWER7)
>>> support, so we really should throw an error when using those options
>>> together.  Ditto for -mabi=ieeelongdouble and -mno-vsx.  The patch below
>>> does that.
>>>
>>> Ok for mainline once bootstrap and regtesting are complete and clean?
>>
>> Okay.  Eventually we shouldn't allow selecting popcntd independently from
>> -mcpu=, but that day isn't here yet.  So, okay for trunk, and backports
>> if wanted.  Thanks!
> 
> Ok, committed to mainline.  It looks like GCC8 needs the same patch.
> I'll have to look closer at GCC7 on whether it needs it too, since the
> code seems to be a little different.

Ok, both GCC8 and GCC7 need the backport.  I committed the backport
to GCC8 and will do the same for GCC7 once the freeze is over.

Peter

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Peter Bergner]

On 11/28/18 3:27 PM, Peter Bergner wrote:
> gcc/
> 	PR target/87496
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disallow
> 	-mabi=ieeelongdouble without both -mpopcntd and -mvsx.

So this "fix" ended up accidentally disallowing -mabi=ibmlongdouble as well,
which is wrong.  I need to look at not only whether the variable
global_options_set.x_rs6000_ieeequad is set, which tells us whether we
used -mabi={ibm,ieee}longdouble, but whether we actually enabled ieee128.
The patch below fixes that oversight.  As we talked offline, we also should
not allow either -mabi={ibm,ieee}longdouble with -mlong-double-64, since
they are conflicting options.  I have added code to test for that as well.
I have also added extra test cases to test for those.

Is this ok for mainline once bootstrap and regtesting come back clean?
Since I backported the earlier fix to GCC8, I'd like to backport this
there too.

Peter


gcc/
	PR target/87496
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disallow
	-mabi=ieeelongdouble and -mabi=ibmlongdouble without -mlong-double-128.
	Do not error for -mabi=ibmlongdouble and no ISA 2.06 support.

gcc/testsuite/
	PR target/87496
	* gcc.target/powerpc/pr87496.c: Rename from this...
	* gcc.target/powerpc/pr87496-1.c: ...to this.  Update comment.
	* gcc.target/powerpc/pr87496-2.c: New test.
	* gcc.target/powerpc/pr87496-3.c: New test.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 266792)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -4282,6 +4282,13 @@ rs6000_option_override_internal (bool gl
     }
   else if (rs6000_long_double_type_size == 128)
     rs6000_long_double_type_size = FLOAT_PRECISION_TFmode;
+  else if (global_options_set.x_rs6000_ieeequad)
+    {
+      if (global_options.x_rs6000_ieeequad)
+	error ("%qs requires %qs", "-mabi=ieeelongdouble", "-mlong-double-128");
+      else
+	error ("%qs requires %qs", "-mabi=ibmlongdouble", "-mlong-double-128");
+    }
 
   /* Set -mabi=ieeelongdouble on some old targets.  In the future, power server
      systems will also set long double to be IEEE 128-bit.  AIX and Darwin
@@ -4293,7 +4300,8 @@ rs6000_option_override_internal (bool gl
 
   else
     {
-      if (!TARGET_POPCNTD || !TARGET_VSX)
+      if (global_options.x_rs6000_ieeequad
+	  && (!TARGET_POPCNTD || !TARGET_VSX))
 	error ("%qs requires full ISA 2.06 support", "-mabi=ieeelongdouble");
 
       if (rs6000_ieeequad != TARGET_IEEEQUAD_DEFAULT && TARGET_LONG_DOUBLE_128)
Index: gcc/testsuite/gcc.target/powerpc/pr87496-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr87496-1.c	(revision 266792)
+++ gcc/testsuite/gcc.target/powerpc/pr87496-1.c	(working copy)
@@ -1,4 +1,4 @@
-/* PR target/87496.c */
+/* PR target/87496 */
 /* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
 /* { dg-skip-if "" { powerpc*-*-darwin* } } */
 /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
Index: gcc/testsuite/gcc.target/powerpc/pr87496-2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr87496-2.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr87496-2.c	(working copy)
@@ -0,0 +1,9 @@
+/* PR target/87496 */
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
+/* { dg-options "-O2 -mcpu=power7 -mabi=ieeelongdouble -mlong-double-64 -Wno-psabi" } */
+
+int i;
+
+/* { dg-error "'-mabi=ieeelongdouble' requires '-mlong-double-128'" "PR87496" { target *-*-* } 0 } */
Index: gcc/testsuite/gcc.target/powerpc/pr87496-3.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr87496-3.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr87496-3.c	(working copy)
@@ -0,0 +1,8 @@
+/* PR target/87496 */
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-options "-O2 -mabi=ibmlongdouble -mlong-double-64 -Wno-psabi" } */
+
+int i;
+
+/* { dg-error "'-mabi=ibmlongdouble' requires '-mlong-double-128'" "PR87496" { target *-*-* } 0 } */
Index: gcc/testsuite/gcc.target/powerpc/pr87496.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr87496.c	(revision 266792)
+++ gcc/testsuite/gcc.target/powerpc/pr87496.c	(nonexistent)
@@ -1,10 +0,0 @@
-/* PR target/87496.c */
-/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
-/* { dg-skip-if "" { powerpc*-*-darwin* } } */
-/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
-/* { dg-require-effective-target longdouble128 } */
-/* { dg-options "-O2 -mcpu=power7 -mabi=ieeelongdouble -mno-popcntd -Wno-psabi" } */
-
-int i;
-
-/* { dg-error "'-mabi=ieeelongdouble' requires full ISA 2.06 support" "PR87496" { target *-*-* } 0 } */

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Segher Boessenkool]

On Tue, Dec 04, 2018 at 01:42:52PM -0600, Peter Bergner wrote:
> On 11/28/18 3:27 PM, Peter Bergner wrote:
> > gcc/
> > 	PR target/87496
> > 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disallow
> > 	-mabi=ieeelongdouble without both -mpopcntd and -mvsx.
> 
> So this "fix" ended up accidentally disallowing -mabi=ibmlongdouble as well,
> which is wrong.  I need to look at not only whether the variable
> global_options_set.x_rs6000_ieeequad is set, which tells us whether we
> used -mabi={ibm,ieee}longdouble, but whether we actually enabled ieee128.
> The patch below fixes that oversight.  As we talked offline, we also should
> not allow either -mabi={ibm,ieee}longdouble with -mlong-double-64, since
> they are conflicting options.  I have added code to test for that as well.
> I have also added extra test cases to test for those.

Okay, so you make -mabi={ibm,ieee}longdouble be valid options only if
there is -mlong-double-128.  Could you add that to the documentation then
please?

> Is this ok for mainline once bootstrap and regtesting come back clean?

Okay with that documentation added and if it tests okay, yes.  Thanks!

> Since I backported the earlier fix to GCC8, I'd like to backport this
> there too.

Okay for there too.


Segher


> gcc/
> 	PR target/87496
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disallow
> 	-mabi=ieeelongdouble and -mabi=ibmlongdouble without -mlong-double-128.
> 	Do not error for -mabi=ibmlongdouble and no ISA 2.06 support.
> 
> gcc/testsuite/
> 	PR target/87496
> 	* gcc.target/powerpc/pr87496.c: Rename from this...
> 	* gcc.target/powerpc/pr87496-1.c: ...to this.  Update comment.
> 	* gcc.target/powerpc/pr87496-2.c: New test.
> 	* gcc.target/powerpc/pr87496-3.c: New test.

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Peter Bergner]

On 12/4/18 4:53 PM, Segher Boessenkool wrote:
> Okay, so you make -mabi={ibm,ieee}longdouble be valid options only if
> there is -mlong-double-128.  Could you add that to the documentation then
> please?

Done.


>> Is this ok for mainline once bootstrap and regtesting come back clean?
> 
> Okay with that documentation added and if it tests okay, yes.  Thanks!

...and committed to trunk.


>> Since I backported the earlier fix to GCC8, I'd like to backport this
>> there too.
> 
> Okay for there too.

Great, I'll backport the changes and commit after regression testing.
Thanks!

Peter

Re: [PATCH, rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046

[Peter Bergner]

On 12/7/18 11:38 AM, Peter Bergner wrote:
> On 12/4/18 4:53 PM, Segher Boessenkool wrote:
>>> Since I backported the earlier fix to GCC8, I'd like to backport this
>>> there too.
>>
>> Okay for there too.
> 
> Great, I'll backport the changes and commit after regression testing.
> Thanks!

Backports committed to GCC 8 and GCC 7 now too.

Peter