| Message ID | fbedc7ce-d1f1-4adf-ff4c-4c0401daa18b@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000] Fix PR87496: ICE in aggregate_value_p at gcc/function.c:2046 | expand |
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
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.
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
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
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 } */
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.
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
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
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 } */