On Tue, Oct 30, 2012 at 10:48 AM, Steven Bosscher <stevenb....@gmail.com> wrote: > On Tue, Oct 30, 2012 at 6:20 AM, Teresa Johnson wrote: >> 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)); > > So why isn't there one? There can't be a fall-through edge from one > section to the other, so cfgrtl.c:fixup_reorder_chain should have > added a barrier here already in the code under the comment: > > /* Now add jumps and labels as needed to match the blocks new > outgoing edges. */ > > Why isn't it doing that for you?
I triggered the same error in 445.gobmk once I applied the thread_prologue_and_epilogue_insns fixes. This is an assert in the dwarf CFI code that complains about a NOTE_INSN_SWITCH_TEXTS_SECTION note not being preceeded by a barrier: gcc -c -o engine/utils.o -DSPEC_CPU -DNDEBUG -DHAVE_CONFIG_H -I. -I.. -I../include -I./include -fprofile-use -freorder-blocks-and-partition -freorder-blocks -ffunction-sections -O2 engine/utils.c engine/utils.c: In function ‘visible_along_edge’: engine/utils.c:274:1: internal compiler error: in create_pseudo_cfg, at dwarf2cfi.c:2742 } ^ In this case the switch section note was inside a BB. What I found was that this was due to several phases going into and back out of cfglayout mode again. In this case it was the compgotos phase. There aren't any computed gotos, but this change occurs during cfg_layout_initialize (in try_optimize_cfg called via cleanup_cfg). Here it merged two (non-contiguous) blocks that had a single-successor/single-predecessor relationship. However, the source block was previously on the section boundary and had a SWITCH note prior. This note is put into the header of the bb when we go into cfglayout mode, and ended up inside the new merged block, which was in any case not on the new border between the hot and cold sections. The correct solution in my opinion is to strip out the SWITCH note every time we enter cfglayout mode after bbro, and then invoke insert_section_boundary_note when leaving cfglayout (if one was found on entry to that cfglayout mode) to reapply it. This fixed not only the 445.gobmk failure, but I also found that I no longer need the above change to insert a barrier when inserting the SWITCH note (although now that I think about it more, it must have been the prologue/epilogue code fix that addressed that). In any case, the 445.gobmk code also showed another bug, that would have been caught by your new patch to verify that cold sections never dominate hot sections. In this case, the func entry block was marked cold and we switch to hot code part way through the function. The reason is that the entry bb count was 2 which is < than the # training runs which is 8. But later on in the code there is a loop which brings those bb counts above 8, and so they are marked hot. Obviously this doesn't make sense. The fix I plan to implement to the bbpart code to ensure no cold blocks dominate hot bbs should address this, but a more sophisticated algorithm for marking blocks hot vs cold would be better (either via the structural method you were working on, or by doing this on traces as part of bbro as David suggested). My plan is to add in your domination check patch, implement the domination fixes in the bbpart algorithm, do a bunch more testing and send the whole shebang out for review. Thanks, Teresa > > BTW, something else I noted in cfgrtl.c: > NOTE_INSN_SWITCH_TEXT_SECTIONS shouldn't be copied in > duplicate_insn_chain, so the following is necessary for robustness: > > Index: cfgrtl.c > =================================================================== > --- cfgrtl.c (revision 191819) > +++ cfgrtl.c (working copy) > @@ -3615,7 +3615,6 @@ > break; > > case NOTE_INSN_EPILOGUE_BEG: > - case NOTE_INSN_SWITCH_TEXT_SECTIONS: > emit_note_copy (insn); > break; > > > There can be only one! One note to rule them all! etc. > > >> 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; > > My dislike for this whole scheme just continues to grow... How can > there be a REG_CROSSING_JUMP note if BB_PARTITION(a)==BB_PARTITION(b)? > That is a bug. We should not need the notes here. > > As long as we have the CFG, BB_PARTITION(a)==BB_PARTITION(b) should be > the canonical way to check whether two blocks are in the same > partition, and the EDGE_CROSSING flag should be set iff an edge > crosses from one section to another. The REG_CROSSING_JUMP note should > only be used to see if a JUMP_INSN may jump to another section, > without having to check all successor edges. > > Any place where we have to check the BB_PARTITION or > edge->flags&EDGE_CROSSING *and* REG_CROSSING_JUMP indicates a bug in > the partitioning updating. > > Another BTW: sched-vis.c doesn't handle REG_CROSSING_JUMP notes so > that slim RTL dumping breaks. I need this patchlet to make things > work: > Index: sched-vis.c > =================================================================== > --- sched-vis.c (revision 191819) > +++ sched-vis.c (working copy) > @@ -553,6 +553,11 @@ > { > char t1[BUF_LEN], t2[BUF_LEN], t3[BUF_LEN]; > > + if (! x) > + { > + sprintf (buf, "(nil)"); > + return; > + } > switch (GET_CODE (x)) > { > case SET: > > Ciao! > Steven -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413