| Message ID | CAAe5K+UGgGzPfbJJCuWeqo2iGfQRd7kCbPFjyhdgrqC02zRJ9Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 21, 2013 at 8:57 PM, Teresa Johnson wrote: > There are two problems I am fixing here (see > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59233 for full analysis). > > The first is the original ICE in crossjump optimization, which was > exposed by enabling -freorder-blocks-and-partition which was > conservatively preventing some upstream block combining optimizations. > The issue is that the block ended in a NOTE_INSN_DELETED, which > old_insns_match_p could not handle. I am fixing this by passing > old_insns_match_p the last instruction in the block that it knows how > to handle. > > The second issue this exposed was that we were unnecessarily marking > landing pad branches EDGE_PRESERVE since > flag_reorder_blocks_and_partition was on, even though this was -Os and > we will gate partitioning off. So we keep an edge to a landing pad... Why is this a problem? > * bb-reorder.c (do_partition_blocks): New function. > (gate_handle_partition_blocks): Call do_partition_blocks. > * bb-reorder.h (do_partition_blocks): Declare. > * except.c (dw2_build_landing_pads): Call do_partition_blocks. Exporting this gate function from bb-reorder.c shouldn't be necessary. Better fix this at the root, in except.c. > * cfgcleanup.c (outgoing_edges_match): Walk up past note instructions > not understood by old_insns_match_p. This part is OK. Ciao! Steven
On Thu, Nov 21, 2013 at 3:31 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Thu, Nov 21, 2013 at 8:57 PM, Teresa Johnson wrote: >> There are two problems I am fixing here (see >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59233 for full analysis). >> >> The first is the original ICE in crossjump optimization, which was >> exposed by enabling -freorder-blocks-and-partition which was >> conservatively preventing some upstream block combining optimizations. >> The issue is that the block ended in a NOTE_INSN_DELETED, which >> old_insns_match_p could not handle. I am fixing this by passing >> old_insns_match_p the last instruction in the block that it knows how >> to handle. >> >> The second issue this exposed was that we were unnecessarily marking >> landing pad branches EDGE_PRESERVE since >> flag_reorder_blocks_and_partition was on, even though this was -Os and >> we will gate partitioning off. > > So we keep an edge to a landing pad... Why is this a problem? It just seems silly to limit optimization unnecessarily when we can deduce that we won't actually attempt partitioning. > > >> * bb-reorder.c (do_partition_blocks): New function. >> (gate_handle_partition_blocks): Call do_partition_blocks. >> * bb-reorder.h (do_partition_blocks): Declare. >> * except.c (dw2_build_landing_pads): Call do_partition_blocks. > > Exporting this gate function from bb-reorder.c shouldn't be necessary. > Better fix this at the root, in except.c. How would we do that? I didn't want to clone the logic that determines whether we will partition, so I thought outlining the checks from the gate function and exporting them would be cleaner. But I am open to suggestions. > > >> * cfgcleanup.c (outgoing_edges_match): Walk up past note instructions >> not understood by old_insns_match_p. > > This part is OK. Ok, let me get this part in first, since it will address the ICE. Thanks, Teresa > > Ciao! > Steven
Index: bb-reorder.c =================================================================== --- bb-reorder.c (revision 205063) +++ bb-reorder.c (working copy) @@ -2532,8 +2532,13 @@ make_pass_duplicate_computed_gotos (gcc::context * return new pass_duplicate_computed_gotos (ctxt); } -static bool -gate_handle_partition_blocks (void) +/* This function will return true if hot/cold partitioning will + be performed, and is declared non-static so that other passes + (namely landing pad expansion in except.c) can more accurately + determine whether they need to assume that partitioning will occur. */ + +bool +do_partition_blocks (void) { /* The optimization to partition hot/cold basic blocks into separate sections of the .o file does not work well with linkonce or with @@ -2548,6 +2553,12 @@ make_pass_duplicate_computed_gotos (gcc::context * && !user_defined_section_attribute); } +static bool +gate_handle_partition_blocks (void) +{ + return do_partition_blocks (); +} + /* This function is the main 'entrance' for the optimization that partitions hot and cold basic blocks into separate sections of the .o file (to improve performance and cache locality). Ideally it Index: bb-reorder.h =================================================================== --- bb-reorder.h (revision 205063) +++ bb-reorder.h (working copy) @@ -37,4 +37,5 @@ extern int get_uncond_jump_length (void); extern void insert_section_boundary_note (void); +extern bool do_partition_blocks (void); #endif Index: cfgcleanup.c =================================================================== --- cfgcleanup.c (revision 205063) +++ cfgcleanup.c (working copy) @@ -1743,12 +1743,20 @@ outgoing_edges_match (int mode, basic_block bb1, b } } + /* Find the last non-debug non-note instruction in each bb, except + stop when we see the NOTE_INSN_BASIC_BLOCK, as old_insns_match_p + handles that case specially. old_insns_match_p does not handle + other types of instruction notes. */ rtx last1 = BB_END (bb1); rtx last2 = BB_END (bb2); - if (DEBUG_INSN_P (last1)) - last1 = prev_nondebug_insn (last1); - if (DEBUG_INSN_P (last2)) - last2 = prev_nondebug_insn (last2); + while (!NOTE_INSN_BASIC_BLOCK_P (last1) && + (DEBUG_INSN_P (last1) || NOTE_P (last1))) + last1 = PREV_INSN (last1); + while (!NOTE_INSN_BASIC_BLOCK_P (last2) && + (DEBUG_INSN_P (last2) || NOTE_P (last2))) + last2 = PREV_INSN (last2); + gcc_assert (last1 && last2); + /* First ensure that the instructions match. There may be many outgoing edges so this test is generally cheaper. */ if (old_insns_match_p (mode, last1, last2) != dir_both) Index: except.c =================================================================== --- except.c (revision 205063) +++ except.c (working copy) @@ -125,6 +125,7 @@ along with GCC; see the file COPYING3. If not see #include "except.h" #include "hard-reg-set.h" #include "basic-block.h" +#include "bb-reorder.h" #include "output.h" #include "dwarf2asm.h" #include "dwarf2out.h" @@ -1014,7 +1015,7 @@ dw2_build_landing_pads (void) new landing pads later, which means that we need to hold on to the post-landing-pad block. Prevent it from being merged away. We'll remove this bit after partitioning. */ - if (flag_reorder_blocks_and_partition) + if (do_partition_blocks ()) e_flags |= EDGE_PRESERVE; for (i = 1; vec_safe_iterate (cfun->eh->lp_array, i, &lp); ++i)
There are two problems I am fixing here (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59233 for full analysis). The first is the original ICE in crossjump optimization, which was exposed by enabling -freorder-blocks-and-partition which was conservatively preventing some upstream block combining optimizations. The issue is that the block ended in a NOTE_INSN_DELETED, which old_insns_match_p could not handle. I am fixing this by passing old_insns_match_p the last instruction in the block that it knows how to handle. The second issue this exposed was that we were unnecessarily marking landing pad branches EDGE_PRESERVE since flag_reorder_blocks_and_partition was on, even though this was -Os and we will gate partitioning off. Added a method for checking whether we will really invoke partitioning. Fixes the original problem (confirmed via a cross-compiler for x86_64-apple-darwin10) and was bootstrapped and tested on x86-64-unknown-linux-gnu. Ok for trunk? Thanks, Teresa 2013-11-21 Teresa Johnson <tejohnson@google.com> * bb-reorder.c (do_partition_blocks): New function. (gate_handle_partition_blocks): Call do_partition_blocks. * bb-reorder.h (do_partition_blocks): Declare. * cfgcleanup.c (outgoing_edges_match): Walk up past note instructions not understood by old_insns_match_p. * except.c (dw2_build_landing_pads): Call do_partition_blocks.