| Message ID | 20121030052000.A4D0F61459@tjsboxrox.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
On 30 October 2012 05:20, Teresa Johnson <tejohnson@google.com> wrote: > Index: cfgrtl.c > =================================================================== > --- cfgrtl.c (revision 192692) > +++ cfgrtl.c (working copy) > @@ -912,7 +912,8 @@ rtl_can_merge_blocks (basic_block a, basic_block b > partition boundaries). See the comments at the top of > bb-reorder.c:partition_hot_cold_basic_blocks for complete details. */ > > - if (BB_PARTITION (a) != BB_PARTITION (b)) > + if (find_reg_note (BB_END (a), REG_CROSSING_JUMP, NULL_RTX) > + || BB_PARTITION (a) != BB_PARTITION (b)) > return false; > > /* Protect the loop latches. */ > @@ -3978,7 +3979,8 @@ cfg_layout_can_merge_blocks_p (basic_block a, basi > partition boundaries). See the comments at the top of > bb-reorder.c:partition_hot_cold_basic_blocks for complete details. */ > > - if (BB_PARTITION (a) != BB_PARTITION (b)) > + if (find_reg_note (BB_END (a), REG_CROSSING_JUMP, NULL_RTX) > + || BB_PARTITION (a) != BB_PARTITION (b)) > return false; > > /* Protect the loop latches. */ As this if() condition seems to be the canonical way to detect being in a different partition should it be moved out into a query function, and all of cfgrtl.c updated to use it? [Note I am not a maintainer and so can't approve/reject your patch]. Thanks, Matt
On Tue, Oct 30, 2012 at 8:49 AM, Matthew Gretton-Dann wrote: > On 30 October 2012 05:20, Teresa Johnson wrote: >> Index: cfgrtl.c >> =================================================================== >> --- cfgrtl.c (revision 192692) >> +++ cfgrtl.c (working copy) >> @@ -912,7 +912,8 @@ rtl_can_merge_blocks (basic_block a, basic_block b >> partition boundaries). See the comments at the top of >> bb-reorder.c:partition_hot_cold_basic_blocks for complete details. */ >> >> - if (BB_PARTITION (a) != BB_PARTITION (b)) >> + if (find_reg_note (BB_END (a), REG_CROSSING_JUMP, NULL_RTX) >> + || BB_PARTITION (a) != BB_PARTITION (b)) >> return false; >> >> /* Protect the loop latches. */ >> @@ -3978,7 +3979,8 @@ cfg_layout_can_merge_blocks_p (basic_block a, basi >> partition boundaries). See the comments at the top of >> bb-reorder.c:partition_hot_cold_basic_blocks for complete details. */ >> >> - if (BB_PARTITION (a) != BB_PARTITION (b)) >> + if (find_reg_note (BB_END (a), REG_CROSSING_JUMP, NULL_RTX) >> + || BB_PARTITION (a) != BB_PARTITION (b)) >> return false; >> >> /* Protect the loop latches. */ > > As this if() condition seems to be the canonical way to detect being > in a different partition should it be moved out into a query function, > and all of cfgrtl.c updated to use it? Not just in cfgrtl.c but for example also in ifcvt.c (which currently only tests for notes, that's broken). Ciao! Steven
Index: function.c =================================================================== --- function.c (revision 192692) +++ function.c (working copy) @@ -6517,7 +6517,7 @@ epilogue_done: basic_block simple_return_block_cold = NULL; edge pending_edge_hot = NULL; edge pending_edge_cold = NULL; - basic_block exit_pred = EXIT_BLOCK_PTR->prev_bb; + basic_block exit_pred; int i; gcc_assert (entry_edge != orig_entry_edge); @@ -6545,6 +6545,12 @@ epilogue_done: else pending_edge_cold = e; } + + /* Save a pointer to the exit's predecessor BB for use in + inserting new BBs at the end of the function. Do this + after the call to split_block above which may split + the original exit pred. */ + exit_pred = EXIT_BLOCK_PTR->prev_bb; FOR_EACH_VEC_ELT (edge, unconverted_simple_returns, i, e) { Index: bb-reorder.c =================================================================== --- bb-reorder.c (revision 192692) +++ bb-reorder.c (working copy) @@ -2188,6 +2188,8 @@ insert_section_boundary_note (void) first_partition = BB_PARTITION (bb); if (BB_PARTITION (bb) != first_partition) { + /* There should be a barrier between text sections. */ + emit_barrier_after (BB_END (bb->prev_bb)); new_note = emit_note_before (NOTE_INSN_SWITCH_TEXT_SECTIONS, BB_HEAD (bb)); /* ??? This kind of note always lives between basic blocks, Index: cfgrtl.c =================================================================== --- cfgrtl.c (revision 192692) +++ cfgrtl.c (working copy) @@ -912,7 +912,8 @@ rtl_can_merge_blocks (basic_block a, basic_block b partition boundaries). See the comments at the top of bb-reorder.c:partition_hot_cold_basic_blocks for complete details. */ - if (BB_PARTITION (a) != BB_PARTITION (b)) + if (find_reg_note (BB_END (a), REG_CROSSING_JUMP, NULL_RTX) + || BB_PARTITION (a) != BB_PARTITION (b)) return false; /* Protect the loop latches. */ @@ -3978,7 +3979,8 @@ cfg_layout_can_merge_blocks_p (basic_block a, basi partition boundaries). See the comments at the top of bb-reorder.c:partition_hot_cold_basic_blocks for complete details. */ - if (BB_PARTITION (a) != BB_PARTITION (b)) + if (find_reg_note (BB_END (a), REG_CROSSING_JUMP, NULL_RTX) + || BB_PARTITION (a) != BB_PARTITION (b)) return false; /* Protect the loop latches. */
This patch fixes three different failures I encountered while trying to use -freorder-blocks-and-partition, including the failure reported in PR 53743. Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? Thanks, Teresa 2012-10-29 Teresa Johnson <tejohnson@google.com> PR optimization/53743 * function.c (thread_prologue_and_epilogue_insns): Don't store exit predecessor BB until after it is potentially split. * bb-reorder.c (insert_section_boundary_note): Ensure that a barrier exists before a switch section node, as this is expected by later passes (e.g. dwarf CFI code). * cfgrtl.c (rtl_can_merge_blocks): Use the same condition looking for region-crossing jumps as in try_redirect_by_replacing_jump, which may be called while merging blocks. (cfg_layout_can_merge_blocks_p): Ditto. -- This patch is available for review at http://codereview.appspot.com/6823047