| Message ID | 877ernpjl9.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFA: Fix PR 68028: LTO error when compiling PowerPC binaries with single precision floating point | expand |
Hi Nick, On Thu, Feb 08, 2018 at 03:49:38PM +0000, Nick Clifton wrote: > I should note that Richard Guenther feels that there is a better way > to solve the problem - by only initializing the values once - but I > still like my solution, so I am offering it here. I thought you were going to do a patch like the following, to make the e500 cores less special (they are not): From 44c3b661ef75e57415b10ca2cec5ac6427bbed8f Mon Sep 17 00:00:00 2001 Message-Id: <44c3b661ef75e57415b10ca2cec5ac6427bbed8f.1518109128.git.segher@kernel.crashing.org> From: Segher Boessenkool <segher@kernel.crashing.org> Date: Thu, 8 Feb 2018 16:58:33 +0000 Subject: [PATCH] 68028 --- gcc/config/rs6000/rs6000.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 5adccd2..73de617 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -4809,25 +4809,6 @@ rs6000_option_override_internal (bool global_init_p) if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET) rs6000_print_isa_options (stderr, 0, "after subtarget", rs6000_isa_flags); - /* For the E500 family of cores, reset the single/double FP flags to let us - check that they remain constant across attributes or pragmas. */ - - switch (rs6000_cpu) - { - case PROCESSOR_PPC8540: - case PROCESSOR_PPC8548: - case PROCESSOR_PPCE500MC: - case PROCESSOR_PPCE500MC64: - case PROCESSOR_PPCE5500: - case PROCESSOR_PPCE6500: - rs6000_single_float = 0; - rs6000_double_float = 0; - break; - - default: - break; - } - if (main_target_opt) { if (main_target_opt->x_rs6000_single_float != rs6000_single_float)
Hi Segher, > I thought you were going to do a patch like the following, to make the > e500 cores less special (they are not): Sorry - my bad. I defer to your patch then. Whatever it takes to get the BZ fixed and off the books... :-) Cheers Nick
Hi! On Fri, Feb 09, 2018 at 09:37:28AM +0000, Nick Clifton wrote: > > I thought you were going to do a patch like the following, to make the > > e500 cores less special (they are not): > > Sorry - my bad. I defer to your patch then. Whatever it takes to get > the BZ fixed and off the books... :-) So does it work? :-) (I cannot reproduce the problem, there must be something in your configuration I'm overlooking). Segher [ Here it is again, with changelog this time: ] From 44c3b661ef75e57415b10ca2cec5ac6427bbed8f Mon Sep 17 00:00:00 2001 Message-Id: <44c3b661ef75e57415b10ca2cec5ac6427bbed8f.1518109128.git.segher@kernel.crashing.org> From: Segher Boessenkool <segher@kernel.crashing.org> Date: Thu, 8 Feb 2018 16:58:33 +0000 Subject: [PATCH] Do not mess with rs6000_{single,double}_float (PR68028) For e500 family cores we do some questionable things with those flags, which does not work with LTO. So don't. 2018-02-10 Segher Boessenkool <segher@kernel.crashing.org> * config/rs6000/rs6000.c (rs6000_option_override_internal): Don't handle rs6000_single_float and rs6000_double_float specially for e500 family CPUs. --- gcc/config/rs6000/rs6000.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 5adccd2..73de617 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -4809,25 +4809,6 @@ rs6000_option_override_internal (bool global_init_p) if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET) rs6000_print_isa_options (stderr, 0, "after subtarget", rs6000_isa_flags); - /* For the E500 family of cores, reset the single/double FP flags to let us - check that they remain constant across attributes or pragmas. */ - - switch (rs6000_cpu) - { - case PROCESSOR_PPC8540: - case PROCESSOR_PPC8548: - case PROCESSOR_PPCE500MC: - case PROCESSOR_PPCE500MC64: - case PROCESSOR_PPCE5500: - case PROCESSOR_PPCE6500: - rs6000_single_float = 0; - rs6000_double_float = 0; - break; - - default: - break; - } - if (main_target_opt) { if (main_target_opt->x_rs6000_single_float != rs6000_single_float)
On 02/09/2018 05:14 PM, Segher Boessenkool wrote: > Hi! > > On Fri, Feb 09, 2018 at 09:37:28AM +0000, Nick Clifton wrote: >>> I thought you were going to do a patch like the following, to make the >>> e500 cores less special (they are not): >> >> Sorry - my bad. I defer to your patch then. Whatever it takes to get >> the BZ fixed and off the books... :-) > > So does it work? :-) (I cannot reproduce the problem, there must be > something in your configuration I'm overlooking). > > > Segher > > > [ Here it is again, with changelog this time: ] > > > From 44c3b661ef75e57415b10ca2cec5ac6427bbed8f Mon Sep 17 00:00:00 2001 > Message-Id: <44c3b661ef75e57415b10ca2cec5ac6427bbed8f.1518109128.git.segher@kernel.crashing.org> > From: Segher Boessenkool <segher@kernel.crashing.org> > Date: Thu, 8 Feb 2018 16:58:33 +0000 > Subject: [PATCH] Do not mess with rs6000_{single,double}_float (PR68028) > > For e500 family cores we do some questionable things with those flags, > which does not work with LTO. So don't. > > > 2018-02-10 Segher Boessenkool <segher@kernel.crashing.org> > > * config/rs6000/rs6000.c (rs6000_option_override_internal): Don't > handle rs6000_single_float and rs6000_double_float specially for > e500 family CPUs. That should work -- the whole issue here is the -mcpu=<whatever> changing other flags. If your assertion is that we need not treat the E500 family of cores special here, then let's just zap that code. Jeff
On Fri, Feb 16, 2018 at 06:05:13PM -0700, Jeff Law wrote: > > From: Segher Boessenkool <segher@kernel.crashing.org> > > Date: Thu, 8 Feb 2018 16:58:33 +0000 > > Subject: [PATCH] Do not mess with rs6000_{single,double}_float (PR68028) > > > > For e500 family cores we do some questionable things with those flags, > > which does not work with LTO. So don't. > > > > > > 2018-02-10 Segher Boessenkool <segher@kernel.crashing.org> > > > > * config/rs6000/rs6000.c (rs6000_option_override_internal): Don't > > handle rs6000_single_float and rs6000_double_float specially for > > e500 family CPUs. > That should work -- the whole issue here is the -mcpu=<whatever> > changing other flags. If your assertion is that we need not treat the > E500 family of cores special here, then let's just zap that code. Okay, I'll commit it soon then (I still cannot actually test it, but yes it is pretty obviously correct; let's hope it solves the original problem as well). Segher
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 257282) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -4834,12 +4834,25 @@ if (main_target_opt) { - if (main_target_opt->x_rs6000_single_float != rs6000_single_float) - error ("target attribute or pragma changes single precision floating " - "point"); - if (main_target_opt->x_rs6000_double_float != rs6000_double_float) - error ("target attribute or pragma changes double precision floating " - "point"); + /* PR 68028: In LTO mode the -mcpu value is passed in as a function + attribute rather than on the command line. So instead of checking + for discrepancioes, we enforce the choice determined by the + attributes. */ + if (in_lto_p) + { + rs6000_single_float = main_target_opt->x_rs6000_single_float; + rs6000_double_float = main_target_opt->x_rs6000_double_float; + } + /* There could be an 'else' statement here but it is hardly worth + it as the compiler will make the optimization anyway, and this + way we avoid indenting the code unnecessarily. */ + + if (main_target_opt->x_rs6000_single_float != rs6000_single_float) + error ("target attribute or pragma changes single precision floating " + "point"); + if (main_target_opt->x_rs6000_double_float != rs6000_double_float) + error ("target attribute or pragma changes double precision floating " + "point"); } rs6000_always_hint = (rs6000_tune != PROCESSOR_POWER4
Hi Segher, OK, here is an official submission of my patch to fix PR 68028. I should note that Richard Guenther feels that there is a better way to solve the problem - by only initializing the values once - but I still like my solution, so I am offering it here. OK to apply ? Cheers Nick gcc/ChangeLog 2018-02-08 Nick Clifton <nickc@redhat.com> * config/rs6000/rs6000.c (rs6000_option_override_internal): In LTO mode prefer function attributes over command line -mcpu setting.