| Message ID | 20150122204317.GM1746@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
> While the cleanup_barriers runs after cleaning up BLOCK_FOR_INSNs, > some targets like i?86/x86_64 choose to populate it again during machine > reorg and some target don't free it at the end of machine reorg. > This patch updates cleanup_barrier pass, so that it adjusts basic block > boundaries and BLOCK_FOR_INSNs in that case, so that we don't crash during > final pass. This isn't a recent regression so what about fixing it more "properly"? For example, by calling free_bb_for_insn at the end of the machinre reorg passes which called compute_bb_for_insn at the beginning? Or do the affected ports need the BB info all the way down to final?
On Mon, Jan 26, 2015 at 10:06:05AM +0100, Eric Botcazou wrote: > > While the cleanup_barriers runs after cleaning up BLOCK_FOR_INSNs, > > some targets like i?86/x86_64 choose to populate it again during machine > > reorg and some target don't free it at the end of machine reorg. > > This patch updates cleanup_barrier pass, so that it adjusts basic block > > boundaries and BLOCK_FOR_INSNs in that case, so that we don't crash during > > final pass. > > This isn't a recent regression so what about fixing it more "properly"? For > example, by calling free_bb_for_insn at the end of the machinre reorg passes > which called compute_bb_for_insn at the beginning? Or do the affected ports > need the BB info all the way down to final? Yes, they do, that is why it crashed during final. Jakub
On Mon, Jan 26, 2015 at 10:11:00AM +0100, Jakub Jelinek wrote: > On Mon, Jan 26, 2015 at 10:06:05AM +0100, Eric Botcazou wrote: > > > While the cleanup_barriers runs after cleaning up BLOCK_FOR_INSNs, > > > some targets like i?86/x86_64 choose to populate it again during machine > > > reorg and some target don't free it at the end of machine reorg. > > > This patch updates cleanup_barrier pass, so that it adjusts basic block > > > boundaries and BLOCK_FOR_INSNs in that case, so that we don't crash during > > > final pass. > > > > This isn't a recent regression so what about fixing it more "properly"? For > > example, by calling free_bb_for_insn at the end of the machinre reorg passes > > which called compute_bb_for_insn at the beginning? Or do the affected ports > > need the BB info all the way down to final? > > Yes, they do, that is why it crashed during final. In particular, all the i386/x86_64 atom tuning AGU stuff depends on it heavily, but e.g. ix86_ok_to_clobber_flags and various others too. Jakub
On Mon, Jan 26, 2015 at 10:11 AM, Jakub Jelinek wrote: > On Mon, Jan 26, 2015 at 10:06:05AM +0100, Eric Botcazou wrote: >> > While the cleanup_barriers runs after cleaning up BLOCK_FOR_INSNs, >> > some targets like i?86/x86_64 choose to populate it again during machine >> > reorg and some target don't free it at the end of machine reorg. >> > This patch updates cleanup_barrier pass, so that it adjusts basic block >> > boundaries and BLOCK_FOR_INSNs in that case, so that we don't crash during >> > final pass. >> >> This isn't a recent regression so what about fixing it more "properly"? For >> example, by calling free_bb_for_insn at the end of the machinre reorg passes >> which called compute_bb_for_insn at the beginning? Or do the affected ports >> need the BB info all the way down to final? > > Yes, they do, that is why it crashed during final. And they should not do so. It cannot be relied upon, in general. Even now, recomputing BLOCK_FOR_INSN only works because machine reorg is the first pass after free_cfg (so nothing has changed yet to the insns stream). Some ports (including x86_64/i386, ia64, and most non-sched_dbr ports) could simply do away with free_cfg and cleanup_barriers. But some ports put things into the insns stream after free_cfg that verify_flow_info chokes on, e.g. the fake insns for const pools for ARM (that causes bugs elsewhere also, see e.g. https://gcc.gnu.org/ml/gcc-patches/2011-07/msg02101.html). The current situation is confusing and bound to give bugs sooner or later... I had patches to have an "early" and "late" free_cfg pass -- I think I even posted them and I still believe that's the right short-term solution -- to make sure nobody can put something between free_cfg and a target with a machine_reorg that needs the CFG, or at least BLOCK_FOR_INSN. Oh well... Ciao! Steven
On Mon, Jan 26, 2015 at 12:35:30PM +0100, Steven Bosscher wrote: > On Mon, Jan 26, 2015 at 10:11 AM, Jakub Jelinek wrote: > > On Mon, Jan 26, 2015 at 10:06:05AM +0100, Eric Botcazou wrote: > >> > While the cleanup_barriers runs after cleaning up BLOCK_FOR_INSNs, > >> > some targets like i?86/x86_64 choose to populate it again during machine > >> > reorg and some target don't free it at the end of machine reorg. > >> > This patch updates cleanup_barrier pass, so that it adjusts basic block > >> > boundaries and BLOCK_FOR_INSNs in that case, so that we don't crash during > >> > final pass. > >> > >> This isn't a recent regression so what about fixing it more "properly"? For > >> example, by calling free_bb_for_insn at the end of the machinre reorg passes > >> which called compute_bb_for_insn at the beginning? Or do the affected ports > >> need the BB info all the way down to final? > > > > Yes, they do, that is why it crashed during final. > > And they should not do so. It cannot be relied upon, in general. Even > now, recomputing BLOCK_FOR_INSN only works because machine reorg is > the first pass after free_cfg (so nothing has changed yet to the insns > stream). > > Some ports (including x86_64/i386, ia64, and most non-sched_dbr ports) > could simply do away with free_cfg and cleanup_barriers. But some > ports put things into the insns stream after free_cfg that > verify_flow_info chokes on, e.g. the fake insns for const pools for > ARM (that causes bugs elsewhere also, see e.g. > https://gcc.gnu.org/ml/gcc-patches/2011-07/msg02101.html). The current > situation is confusing and bound to give bugs sooner or later... > > I had patches to have an "early" and "late" free_cfg pass -- I think I > even posted them and I still believe that's the right short-term > solution -- to make sure nobody can put something between free_cfg and > a target with a machine_reorg that needs the CFG, or at least > BLOCK_FOR_INSN. Sure, we have targets which need cfg late, and targets which don't, and targets which do need it only diring machine reorg and not afterwards. pass_free_cfg doesn't do just free_bb_for_insn though, it also adds a note for hot cold partitioning, and for DBR targets other stuff, though DBR targets are likely helpless with keeping CFG till the end. What my patch does is still compatible with removing that free_bb_for_insn call from pass_free_cfg::execute if some flag is set in targetm, it teaches the pass to handle cfg if it is around. I agree that freeing the cfg and immediately computing it again doesn't make sense, but I just don't see this patch being incompatible with that. Jakub
On Mon, Jan 26, 2015 at 12:47 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Jan 26, 2015 at 12:35:30PM +0100, Steven Bosscher wrote: >> On Mon, Jan 26, 2015 at 10:11 AM, Jakub Jelinek wrote: >> > On Mon, Jan 26, 2015 at 10:06:05AM +0100, Eric Botcazou wrote: >> >> > While the cleanup_barriers runs after cleaning up BLOCK_FOR_INSNs, >> >> > some targets like i?86/x86_64 choose to populate it again during machine >> >> > reorg and some target don't free it at the end of machine reorg. >> >> > This patch updates cleanup_barrier pass, so that it adjusts basic block >> >> > boundaries and BLOCK_FOR_INSNs in that case, so that we don't crash during >> >> > final pass. >> >> >> >> This isn't a recent regression so what about fixing it more "properly"? For >> >> example, by calling free_bb_for_insn at the end of the machinre reorg passes >> >> which called compute_bb_for_insn at the beginning? Or do the affected ports >> >> need the BB info all the way down to final? >> > >> > Yes, they do, that is why it crashed during final. >> >> And they should not do so. It cannot be relied upon, in general. Even >> now, recomputing BLOCK_FOR_INSN only works because machine reorg is >> the first pass after free_cfg (so nothing has changed yet to the insns >> stream). >> >> Some ports (including x86_64/i386, ia64, and most non-sched_dbr ports) >> could simply do away with free_cfg and cleanup_barriers. But some >> ports put things into the insns stream after free_cfg that >> verify_flow_info chokes on, e.g. the fake insns for const pools for >> ARM (that causes bugs elsewhere also, see e.g. >> https://gcc.gnu.org/ml/gcc-patches/2011-07/msg02101.html). The current >> situation is confusing and bound to give bugs sooner or later... >> >> I had patches to have an "early" and "late" free_cfg pass -- I think I >> even posted them and I still believe that's the right short-term >> solution -- to make sure nobody can put something between free_cfg and >> a target with a machine_reorg that needs the CFG, or at least >> BLOCK_FOR_INSN. > > Sure, we have targets which need cfg late, and targets which don't, and > targets which do need it only diring machine reorg and not afterwards. > > pass_free_cfg doesn't do just free_bb_for_insn though, it also adds > a note for hot cold partitioning, and for DBR targets other stuff, though > DBR targets are likely helpless with keeping CFG till the end. > > What my patch does is still compatible with removing that > free_bb_for_insn call from pass_free_cfg::execute if some flag is set > in targetm, it teaches the pass to handle cfg if it is around. > > I agree that freeing the cfg and immediately computing it again doesn't make > sense, but I just don't see this patch being incompatible with that. I wonder if handing over pass pipeline control to targets at machine_reorg time (including free_cfg) makes sense... (either giving control back for stuff like final() or make it call into the middle-end again). Richard. > Jakub
On Mon, Jan 26, 2015 at 01:11:01PM +0100, Richard Biener wrote: > > I agree that freeing the cfg and immediately computing it again doesn't make > > sense, but I just don't see this patch being incompatible with that. > > I wonder if handing over pass pipeline control to targets at > machine_reorg time (including free_cfg) makes sense... > (either giving control back for stuff like final() or make it call > into the middle-end again). Perhaps, but not sure if such changes are desirable for stage4. And certainly unsuitable for release branches. Jakub
On 01/26/15 06:34, Jakub Jelinek wrote: > On Mon, Jan 26, 2015 at 01:11:01PM +0100, Richard Biener wrote: >>> I agree that freeing the cfg and immediately computing it again doesn't make >>> sense, but I just don't see this patch being incompatible with that. >> >> I wonder if handing over pass pipeline control to targets at >> machine_reorg time (including free_cfg) makes sense... >> (either giving control back for stuff like final() or make it call >> into the middle-end again). > > Perhaps, but not sure if such changes are desirable for stage4. > And certainly unsuitable for release branches. That would seem to take a big mistake (machine_reorg) and make it even worse. Jeff
> Yes, they do, that is why it crashed during final.
OK. Why wouldn't it work to call reorder_insns instead of reorder_insns_nobb?
On Tue, Jan 27, 2015 at 09:25:32AM +0100, Eric Botcazou wrote: > > Yes, they do, that is why it crashed during final. > > OK. Why wouldn't it work to call reorder_insns instead of reorder_insns_nobb? Because reorder_insns doesn't handle the case of moving a barrier into a middle of basic block. if (!BARRIER_P (from) && (bb2 = BLOCK_FOR_INSN (from))) { if (BB_END (bb2) == to) BB_END (bb2) = prev; df_set_bb_dirty (bb2); } if (BB_END (bb) == after) BB_END (bb) = to; for (x = from; x != NEXT_INSN (to); x = NEXT_INSN (x)) if (!BARRIER_P (x)) df_insn_change_bb (x, bb); from == to is a BARRIER in this case, BB_END (bb) != after (BB_END is actually PREV_INSN (from)), so this doesn't do anything at all. While what we need is: 1) set BB_END to after 2) clear BLOCK_FOR_INSN on the notes after AFTER (after addition of barrier after FROM == TO) until former PREV_INSN (FROM) (inclusive) Jakub
> Because reorder_insns doesn't handle the case of moving a barrier into a > middle of basic block. Right, I should have read the audit trail. :-) The patch is OK then, but add a ??? note at the end of the comment saying that the proper thing to do here is probably not to run cleanup_barrier for this back-end.
--- gcc/jump.c.jj 2015-01-15 20:25:30.000000000 +0100 +++ gcc/jump.c 2015-01-22 16:32:11.677709401 +0100 @@ -168,7 +168,27 @@ cleanup_barriers (void) if (BARRIER_P (prev)) delete_insn (insn); else if (prev != PREV_INSN (insn)) - reorder_insns_nobb (insn, insn, prev); + { + basic_block bb = BLOCK_FOR_INSN (prev); + rtx_insn *end = PREV_INSN (insn); + reorder_insns_nobb (insn, insn, prev); + if (bb) + { + /* If the backend called in machine reorg compute_bb_for_insn + and didn't free_bb_for_insn again, preserve basic block + boundaries. Move the end of basic block to PREV since + it is followed by a barrier now, and clear BLOCK_FOR_INSN + on the following notes. */ + BB_END (bb) = prev; + do + { + prev = NEXT_INSN (prev); + if (prev != insn && BLOCK_FOR_INSN (prev) == bb) + BLOCK_FOR_INSN (prev) = NULL; + } + while (prev != end); + } + } } } return 0; --- gcc/testsuite/gcc.dg/pr61058.c.jj 2015-01-22 16:19:16.507023479 +0100 +++ gcc/testsuite/gcc.dg/pr61058.c 2015-01-22 16:19:16.507023479 +0100 @@ -0,0 +1,10 @@ +/* PR rtl-optimization/61058 */ +/* { dg-do compile } */ +/* { dg-options "" } */ +/* { dg-additional-options "-fno-asynchronous-unwind-tables -mtune=atom" { target i?86-*-* x86_64-*-* } } */ + +void +foo (void) +{ + __builtin_unreachable (); +}