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? > > 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;
Shouldn't the patch be: @@ -3630,10 +3631,11 @@ duplicate_insn_chain (rtx from, rtx to) case NOTE_INSN_FUNCTION_BEG: /* There is always just single entry to function. */ case NOTE_INSN_BASIC_BLOCK: + /* We should only switch text sections once. */ + case NOTE_INSN_SWITCH_TEXT_SECTIONS: break; case NOTE_INSN_EPILOGUE_BEG: - case NOTE_INSN_SWITCH_TEXT_SECTIONS: emit_note_copy (insn); break; i.e. move the NOTE above to where we will ignore it. Otherwise, we would fall into the default case which is listed as unreachable. Teresa > > > 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