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?
>
> 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 | tejohnson@google.com | 408-460-2413