Re: [PATCH] Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047)

[Teresa Johnson <tejohnson at google dot com>]

On Tue, Oct 30, 2012 at 10:48 AM, Steven Bosscher <stevenb.gcc@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 | tejohnson@google.com | 408-460-2413