| Message ID | ac86cc08-c36f-5929-ee0d-8fbf740a892d@gjlay.de |
|---|---|
| State | New |
| Headers | show |
| Series | [avr] PR92606: Disable -fipa-icf-variables because it generates wrong code. | expand |
Ping #1. > Hi, this patch turns off -fipa-icf-variables because it generates wrong > code like for PR92606. As there is no target hook that could decide > whether such optimizations are obsolete, disable such optimizations > alltogether until PR92932 (target hook to disable such optimizations > depending on object attributes and address-spcace) is available. > > Ok to apply? > > Johann > > > Work around PR ipa/92932 by disabling -fipa-icf-variables until > PR92932 will have been solved. > > PR ipa/92932 > PR target/92606 > * common/config/avr/avr-common.c (avr_option_optimization_table) > <-fipa-icf-variables>: Disable. >
Ping #2. > Hi, this patch turns off -fipa-icf-variables because it generates wrong > code like for PR92606. As there is no target hook that could decide > whether such optimizations are obsolete, disable such optimizations > alltogether until PR92932 (target hook to disable such optimizations > depending on object attributes and address-spcace) is available. > > Ok to apply? > > Johann > > > Work around PR ipa/92932 by disabling -fipa-icf-variables until > PR92932 will have been solved. > > PR ipa/92932 > PR target/92606 > * common/config/avr/avr-common.c (avr_option_optimization_table) > <-fipa-icf-variables>: Disable. >
On Wed, 2019-12-18 at 16:30 +0100, Georg-Johann Lay wrote: > Hi, this patch turns off -fipa-icf-variables because it generates wrong > code like for PR92606. As there is no target hook that could decide > whether such optimizations are obsolete, disable such optimizations > alltogether until PR92932 (target hook to disable such optimizations > depending on object attributes and address-spcace) is available. > > Ok to apply? > > Johann > > > Work around PR ipa/92932 by disabling -fipa-icf-variables until > PR92932 will have been solved. > > PR ipa/92932 > PR target/92606 > * common/config/avr/avr-common.c (avr_option_optimization_table) > <-fipa-icf-variables>: Disable. This seems backwards to me. Instead of disabling the optimization in the target files we should prevent the optimization from firing in cases where it can't reasonably work. Jeff
Jeff Law schrieb: > On Wed, 2019-12-18 at 16:30 +0100, Georg-Johann Lay wrote: >> Hi, this patch turns off -fipa-icf-variables because it generates wrong >> code like for PR92606. As there is no target hook that could decide >> whether such optimizations are obsolete, disable such optimizations >> alltogether until PR92932 (target hook to disable such optimizations >> depending on object attributes and address-spcace) is available. >> >> Ok to apply? >> >> Johann >> >> >> Work around PR ipa/92932 by disabling -fipa-icf-variables until >> PR92932 will have been solved. >> >> PR ipa/92932 >> PR target/92606 >> * common/config/avr/avr-common.c (avr_option_optimization_table) >> <-fipa-icf-variables>: Disable. > This seems backwards to me. Instead of disabling the optimization in > the target files we should prevent the optimization from firing in > cases where it can't reasonably work. > > Jeff The chances that this will be fixed are... tiny. As Andrew notes in a comment to PR92932, there are at least 2 other PRs that report wrong-code due to similar data optimization. He mentions different passes however. Whatever passes perform such wrong-code transforms, apart from more conservative approach they will need a new target hook to properly fix PR92606 because target attributes / address spaces are involved. I'd highly appreciate correct code, even if it's at the expense of (yah, yet another) hack in the avr backend. In particular, because such optimizations will improve code only a tiny little bit -- if at all. Hence kicking out the culprit does not reduce code quality, also because IF such merging is legitimate, some cases can be catched by the linker with, say -fmerge-all-constants. If PR92932, PR92294, PR954666 will ever be fixed, I'd gladly remove the proposed 1-line disable-culprit-hack and implement the new target hook that PR92932 is supposed to bring. Johann
Index: common/config/avr/avr-common.c =================================================================== --- common/config/avr/avr-common.c (revision 279522) +++ common/config/avr/avr-common.c (working copy) @@ -38,6 +38,14 @@ static const struct default_options avr_ { OPT_LEVELS_ALL, OPT_fcaller_saves, NULL, 0 }, { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_mgas_isr_prologues, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_mmain_is_OS_task, NULL, 1 }, + // FIXME: IPA incorrectly identifies variables in .progmem.data (accessed + // via LPM) with variables in .rodata (accessed via LD, LDD, LDS) like + // in PR92606. As there is no target hook to disable such optimizations + // depending on target attributes and / or address-spaces of the involved + // objects (filed as PR92932), ditch such malicious optimizations now until + // PR92932 is implemented and we can use that target hook to solve PR92606 + // properly. + { OPT_LEVELS_ALL, OPT_fipa_icf_variables, NULL, 0 }, { OPT_LEVELS_NONE, 0, NULL, 0 } };