| Message ID | AANLkTinQTxLLJqxgc2UYLBYnuJtLRwZgS14YJ8yLREgA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 18, 2010 at 2:15 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > 2010-11-18 Uros Bizjak <ubizjak@gmail.com> > > PR middle-end/46546 > * passes.c (init_optimization_passes): Move machine_reorg pass before > free_cfg pass. > > Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches? I don't think this will work because machine_reorg on some target recreate the BB's anyways. I think x86 should do that. -- Pinski
On Thu, Nov 18, 2010 at 11:15:47PM +0100, Uros Bizjak wrote: > On Thu, Nov 18, 2010 at 8:43 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > > On Thu, Nov 18, 2010 at 8:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > The fix is then trivial. Not so much. > > 2010-11-18 Uros Bizjak <ubizjak@gmail.com> > > PR middle-end/46546 > * passes.c (init_optimization_passes): Move machine_reorg pass before > free_cfg pass. > > Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches? I'm afraid this is going to break various targets, such change can't be taken lightly and testing just on 2 targets is definitely not sufficient. Definitely not something that should be applied ever to release branches, not sure if it is something that should be done in stage3 for 4.6. Targets that need cfg in the reorg pass compute it themselves (e.g. ia64), other targets could depend on that the CFG is gone. Why does i?86 actually care about CFG in its reorg pass, unlike targets that do scheduling etc. I don't see why it should care. E.g. ia64's comments say: static void ia64_reorg (void) { /* We are freeing block_for_insn in the toplev to keep compatibility with old MDEP_REORGS that are not CFG based. Recompute it now. */ compute_bb_for_insn (); ... I'd say that applying this patch after testing/converting all targets would be nice thing to do for stage1. Jakub
On 11/18/2010 02:15 PM, Uros Bizjak wrote:
> Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches?
Definitely not, as mentioned elsewhere.
I just thought that I'd add that, for next stage1, it would be nice
to have a targetm boolean that says whether pass_machine_reorg needs
the cfg. Because it does seem silly to free the cfg only to have
to immeidately re-compute it.
But that's not something to fix now.
r~
On Thu, Nov 18, 2010 at 11:24 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> 2010-11-18 Uros Bizjak <ubizjak@gmail.com> >> >> PR middle-end/46546 >> * passes.c (init_optimization_passes): Move machine_reorg pass before >> free_cfg pass. >> >> Tested on x86_64-pc-linux-gnu {,-m32}. OK for mainline and release branches? > > I'm afraid this is going to break various targets, such change can't be > taken lightly and testing just on 2 targets is definitely not sufficient. > > Definitely not something that should be applied ever to release branches, > not sure if it is something that should be done in stage3 for 4.6. Note taken. > Targets that need cfg in the reorg pass compute it themselves (e.g. ia64), > other targets could depend on that the CFG is gone. > > Why does i?86 actually care about CFG in its reorg pass, unlike targets > that do scheduling etc. I don't see why it should care. Just for the sole delete_insn of the insn at the BB_END in ix86_pad_returns. We can in fact manually update BB_END, as H.J. proposed. Uros.
> On Thu, Nov 18, 2010 at 8:43 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > > On Thu, Nov 18, 2010 at 8:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > > > >>>> ix86_pad_returns forgot to update BB_END when it > >>>> replaces it with a new one. OK for trunk? > >>> > >>> IMO, you should just move the call to vzeroupper optimization to be > >>> the first thing in ix86_reorg. This way, ix86_pad_short_function, > >>> ix86_pad_returns and ix86_avoid_jump_mispredict will also count > >>> emitted vzeroupper. > >> > >> But it will leave bad BB_END in place. Any uses of BB_END later > >> will still be screwed. > > > > I think that Jan or Richard (CC'd) can provide better answer on this issue. > > Got it. > > It is a pass ordering problem, we free CFG before machine reorg pass, > so BLOCK_FOR_INSN in machine reorg pass does not work anymore (it > returns 0). If the question was why machine reorg runs after cfg freeing, the reason is that all the machine reorgs except for Itanium one (at least last time I looked) were not aware of CFG. I never felt like getting all of them updated as it is bit tricky (they do ugly stuff like inserting constant pool right into code that has CFG representation issues) Honza
Index: passes.c =================================================================== --- passes.c (revision 166920) +++ passes.c (working copy) @@ -1051,8 +1051,8 @@ init_optimization_passes (void) NEXT_PASS (pass_compute_alignments); NEXT_PASS (pass_duplicate_computed_gotos); NEXT_PASS (pass_variable_tracking); - NEXT_PASS (pass_free_cfg); NEXT_PASS (pass_machine_reorg); + NEXT_PASS (pass_free_cfg); NEXT_PASS (pass_cleanup_barriers); NEXT_PASS (pass_delay_slots); NEXT_PASS (pass_split_for_shorten_branches);