| Message ID | 4F44C5ED.1030002@gjlay.de |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 22, 2012 at 11:39 AM, Georg-Johann Lay <avr@gjlay.de> wrote: > Denis Chertykov wrote: >> 2012/2/21 Richard Henderson: >>> On 02/21/12 09:08, Georg-Johann Lay wrote: >>>> PR rtl-optimization/50063 >>>> * config/avr/avr.md (movhi_sp_r): Handle -1 (unknown IRQ state) >>>> and 2 (8-bit SP) in operand 2. >>>> * config/avr/avr.c (avr_prologue_setup_frame): Adjust prologue >>>> setup to use movhi_sp_r instead of vanilla move to write SP. >>>> Adjust REG_CFA notes to superseed unspec. >>>> (expand_epilogue): Adjust epilogue setup to use read_sp instead >>>> of vanilla move. >>>> As function body might contain CLI or SEI: Use irq_state 0 (IRQ >>>> known to be off) only with TARGET_NO_INTERRUPTS. Never use >>>> irq_state 1 (IRQ known to be on) here. >>> The CFA bits in avr_prologue_setup_frame look good. >>> I'll let Denis or Eric review the movhi_sp_r change. >> >> Approved. >> >> Denis. > > Here is a patchlet for 4.6. It just sets -fno-dse to work around the problem > because I think back-porting all what's needed is too much change. > > Compiling avr-libc without DSE gives the same sizes for all objects which shows > that this is not a crucial optimization for avr. > > Ok for 4.6 branch? I don't think this kind of fixes are wanted. The patch misses a testcase to backport and the issue surely not only affects DSE. Richard. > Johann > > > PR rtl-optimization/50063 > * config/avr/avr.c (avr_option_override): Disable DSE. > > > > Index: config/avr/avr.c > =================================================================== > --- config/avr/avr.c (revision 184460) > +++ config/avr/avr.c (working copy) > @@ -245,6 +245,11 @@ avr_option_override (void) > > flag_delete_null_pointer_checks = 0; > > + /* Kick off DSE in order to hack around PR rtl-optimization/50063. > + Backporting all of 4.7 is too much. */ > + > + flag_dse = 0; > + > for (t = avr_mcu_types; t->name; t++) > if (strcmp (t->name, avr_mcu_name) == 0) > break; >
Richard Guenther wrote: > On Wed, Feb 22, 2012 at 11:39 AM, Georg-Johann Lay <avr@gjlay.de> wrote: >> Denis Chertykov wrote: >>> 2012/2/21 Richard Henderson: >>>> On 02/21/12 09:08, Georg-Johann Lay wrote: >>>>> PR rtl-optimization/50063 >>>>> * config/avr/avr.md (movhi_sp_r): Handle -1 (unknown IRQ state) >>>>> and 2 (8-bit SP) in operand 2. >>>>> * config/avr/avr.c (avr_prologue_setup_frame): Adjust prologue >>>>> setup to use movhi_sp_r instead of vanilla move to write SP. >>>>> Adjust REG_CFA notes to superseed unspec. >>>>> (expand_epilogue): Adjust epilogue setup to use read_sp instead >>>>> of vanilla move. >>>>> As function body might contain CLI or SEI: Use irq_state 0 (IRQ >>>>> known to be off) only with TARGET_NO_INTERRUPTS. Never use >>>>> irq_state 1 (IRQ known to be on) here. >>>> The CFA bits in avr_prologue_setup_frame look good. >>>> I'll let Denis or Eric review the movhi_sp_r change. >>> Approved. >>> >>> Denis. >> Here is a patchlet for 4.6. It just sets -fno-dse to work around the problem >> because I think back-porting all what's needed is too much change. >> >> Compiling avr-libc without DSE gives the same sizes for all objects which shows >> that this is not a crucial optimization for avr. >> >> Ok for 4.6 branch? > > I don't think this kind of fixes are wanted. The patch misses a > testcase to backport > and the issue surely not only affects DSE. > > Richard. Actually, hacking the backend to work around assumptions in rtl-optimizers by hiding information in unspec_volatile is not wanted, too. If I understand correctly, no one is inclined to fix the root cause of the bug, i.e. don't let RTL optimizers make assumptions that shred some backends. Johann
On Wed, Feb 22, 2012 at 1:11 PM, Georg-Johann Lay <avr@gjlay.de> wrote: > Richard Guenther wrote: >> On Wed, Feb 22, 2012 at 11:39 AM, Georg-Johann Lay <avr@gjlay.de> wrote: >>> Denis Chertykov wrote: >>>> 2012/2/21 Richard Henderson: >>>>> On 02/21/12 09:08, Georg-Johann Lay wrote: >>>>>> PR rtl-optimization/50063 >>>>>> * config/avr/avr.md (movhi_sp_r): Handle -1 (unknown IRQ state) >>>>>> and 2 (8-bit SP) in operand 2. >>>>>> * config/avr/avr.c (avr_prologue_setup_frame): Adjust prologue >>>>>> setup to use movhi_sp_r instead of vanilla move to write SP. >>>>>> Adjust REG_CFA notes to superseed unspec. >>>>>> (expand_epilogue): Adjust epilogue setup to use read_sp instead >>>>>> of vanilla move. >>>>>> As function body might contain CLI or SEI: Use irq_state 0 (IRQ >>>>>> known to be off) only with TARGET_NO_INTERRUPTS. Never use >>>>>> irq_state 1 (IRQ known to be on) here. >>>>> The CFA bits in avr_prologue_setup_frame look good. >>>>> I'll let Denis or Eric review the movhi_sp_r change. >>>> Approved. >>>> >>>> Denis. >>> Here is a patchlet for 4.6. It just sets -fno-dse to work around the problem >>> because I think back-porting all what's needed is too much change. >>> >>> Compiling avr-libc without DSE gives the same sizes for all objects which shows >>> that this is not a crucial optimization for avr. >>> >>> Ok for 4.6 branch? >> >> I don't think this kind of fixes are wanted. The patch misses a >> testcase to backport >> and the issue surely not only affects DSE. >> >> Richard. > > Actually, hacking the backend to work around assumptions in rtl-optimizers by > hiding information in unspec_volatile is not wanted, too. > > If I understand correctly, no one is inclined to fix the root cause of the bug, > i.e. don't let RTL optimizers make assumptions that shred some backends. I'm not sure about this. Certainly you are the one hitting this bug(?), so you would be the perfect volunteer to fix it ;) Richard. > Johann
Index: config/avr/avr.c =================================================================== --- config/avr/avr.c (revision 184460) +++ config/avr/avr.c (working copy) @@ -245,6 +245,11 @@ avr_option_override (void) flag_delete_null_pointer_checks = 0; + /* Kick off DSE in order to hack around PR rtl-optimization/50063. + Backporting all of 4.7 is too much. */ + + flag_dse = 0; + for (t = avr_mcu_types; t->name; t++) if (strcmp (t->name, avr_mcu_name) == 0) break;