Re: Fix PR 53743 and other -freorder-blocks-and-partition failures

[Teresa Johnson <tejohnson at google dot com>]

On Mon, May 20, 2013 at 3:42 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Mon, May 20, 2013 at 4:55 AM, Teresa Johnson wrote:
>
>>         * ifcvt.c (find_if_case_1): Replace BB_COPY_PARTITION with assert
>>         as this is now done by redirect_edge_and_branch_force.
>>         * function.c (thread_prologue_and_epilogue_insns): Insert new bb after
>>         barriers, and fix interaction with splitting.
>>         * emit-rtl.c (try_split): Copy REG_CROSSING_JUMP notes.
>>         * cfgcleanup.c (try_forward_edges): Fix early return value to properly
>>         reflect changes made in the routine.
>>         * bb-reorder.c (emit_barrier_after_bb): Handle insertion in
>>         non-cfglayout mode.
>>         (fix_up_fall_thru_edges): Remove incorrect check for bb layout order
>>         since this is called in cfglayout mode, and replace partition fixup
>>         with assert as that is now done by force_nonfallthru_and_redirect.
>>         (add_reg_crossing_jump_notes): Handle the fact that some jumps may
>>         already be marked with region crossing note.
>>         (insert_section_boundary_note): Make non-static, gate on flag
>>         has_bb_partition, rewrite to also check for multiple partitions.
>>         (rest_of_handle_reorder_blocks): Remove call to
>>         insert_section_boundary_note, now done later during free_cfg.
>>         * bb-reorder.h: Declare insert_section_boundary_note and
>>         emit_barrier_after_bb, which are no longer static.
>>         * Makefile.in (cfgrtl.o): Depend on bb-reorder.h
>>         * cfgrtl.c (rest_of_pass_free_cfg): If partitions exist
>>         invoke insert_section_boundary_note.
>>         (try_redirect_by_replacing_jump): Remove unnecessary
>>         check for region crossing note.
>>         (fixup_partition_crossing): New function.
>>         (rtl_redirect_edge_and_branch): Fixup partition boundaries.
>>         (force_nonfallthru_and_redirect): Fixup partition boundaries,
>>         remove old code that tried to do this. Emit barrier correctly
>>         when we are in cfglayout mode.
>>         (rtl_split_edge): Correctly fixup partition boundaries.
>>         (commit_one_edge_insertion): Remove old code that tried to
>>         fixup region crossing edge since this is now handled in
>>         split_block, and set up insertion point correctly since
>>         block may now end in a jump.
>>         (rtl_verify_edges): Add checks for incorrect/missing REG_CROSSING_JUMP
>>         notes.
>>         (fixup_reorder_chain): Remove old code that attempted to fixup region
>>         crossing note as this is now handled in force_nonfallthru_and_redirect.
>>         (duplicate_insn_chain): Don't duplicate switch section notes.
>>         (rtl_can_remove_branch_p): Remove unnecessary check for region crossing
>>         note.
>>
>> Index: ifcvt.c
>> ===================================================================
>> --- ifcvt.c     (revision 199014)
>> +++ ifcvt.c     (working copy)
>> @@ -3905,10 +3905,9 @@ find_if_case_1 (basic_block test_bb, edge then_edg
>>    if (new_bb)
>>      {
>>        df_bb_replace (then_bb_index, new_bb);
>> -      /* Since the fallthru edge was redirected from test_bb to new_bb,
>> -         we need to ensure that new_bb is in the same partition as
>> -         test bb (you can not fall through across section boundaries).  */
>> -      BB_COPY_PARTITION (new_bb, test_bb);
>> +      /* This should have been done above via force_nonfallthru_and_redirect
>> +         (possibly called from redirect_edge_and_branch_force).  */
>> +      gcc_assert (BB_PARTITION (new_bb) == BB_PARTITION (test_bb));
>>      }
>
> Nit: gcc_checking_assert() ?

ok.

>
>
>> Index: function.c
>> ===================================================================
>> --- function.c  (revision 199014)
>> +++ function.c  (working copy)
>> @@ -6270,8 +6270,10 @@ thread_prologue_and_epilogue_insns (void)
>>                     break;
>>                 if (e)
>>                   {
>> -                   copy_bb = create_basic_block (NEXT_INSN (BB_END (e->src)),
>> -                                                 NULL_RTX, e->src);
>> +                    /* Make sure we insert after any barriers.  */
>> +                    rtx end = get_last_bb_insn (e->src);
>> +                    copy_bb = create_basic_block (NEXT_INSN (end),
>> +                                                  NULL_RTX, e->src);
>>                     BB_COPY_PARTITION (copy_bb, e->src);
>>                   }
>>                 else
>
> This is a nice catch. The change at first glance looks unrelated to
> the profiling fixes, but apparently you have a test case where e is
> not an EDGE_FALLTHRU edge? Otherwise there can't be a barrier. The
> change looks OK to me.

Yes, according to my notes this fix was from a case in hmmer where the
edge was a crossing edge.

>
>
>> Index: bb-reorder.c
>> ===================================================================
>> --- bb-reorder.c        (revision 199014)
>> +++ bb-reorder.c        (working copy)
>> @@ -1380,13 +1380,16 @@ get_uncond_jump_length (void)
>>    return length;
>>  }
>>
>> -/* Emit a barrier into the footer of BB.  */
>> +/* Emit a barrier after BB, into the footer if we are in CFGLAYOUT mode.  */
>>
>> -static void
>> +void
>>  emit_barrier_after_bb (basic_block bb)
>>  {
>
> I still think this function should just be moved out of bb-reorder.c
> then. Move it to cfgrtl,c please, and prototype it in basic-block.h
> (one day we'll have to split basic-block.h into rtl/gimple/generic
> parts, TODO++).
>
> But to be honest, I still don't really understand why we emit a
> barrier at all if we're in cfglayout mode. They're ignored, they're
> going to be overlooked if someone looks for barriers after basic
> blocks (nobody looks in the footer, and they should not), and
> fixup_reorder_chain ought to put barriers where needed when coming out
> of cfglayout mode. (TODO++ - that's how it goes for me every time I
> look at gcc code ;-)

Ok, I've moved it. Interestingly, previous to my modification to
enable this to be called in non-cfglayout mode it was only called from
bbpart, which is in cfglayout mode. This may be another case where
bbpart is confused about what mode it is in? Probably worth some
investigation/cleanup at some point as you note.

>
>
>>         {
>> -         emit_note_before (NOTE_INSN_SWITCH_TEXT_SECTIONS, BB_HEAD (bb));
>> -         break;
>> +         if (switched_sections)
>> +           {
>> +             error ("multiple hot/cold transitions found (bb %i)",
>> +                    bb->index);
>> +             err = 1;
>
> Just gcc_assert (! switched_sections).

The rest of the rtl flow verification code used error () and returned
an err=1, so I was trying to be consistent with that. Do we want to be
different in this routine?

>
>
>> @@ -2180,8 +2199,6 @@ rest_of_handle_reorder_blocks (void)
>>        bb->aux = bb->next_bb;
>>    cfg_layout_finalize ();
>>
>> -  /* Add NOTE_INSN_SWITCH_TEXT_SECTIONS notes.  */
>> -  insert_section_boundary_note ();
>>    return 0;
>>  }
>
> ...and the titans were defeated.
>
>
>> +      /* Remove the region crossing note from jump at end of
>
> s/region/section/
>
> Looks good to me, overall. The only thing missing is test cases, if
> you have any.

I manually ran all of the gcc.c-torture/execute tests to collect and
feedback fdo, and enable -freorder-blocks-and-partitions, with and
without my fixes. This gave me a few test cases that I will add as
fdo/partitioning test cases. A couple were still problems that I
hadn't flushed out yet, relating to the new verification code that
checks that we only have a single section switch in the function,
because a couple post-bbro phases (e.g. compgoto) were not being
careful enough.

Revised patch under testing and coming tomorrow.

>
> Can you file a bug report for the debug info issues?

Will do.

Thanks,
Teresa

>
> Ciao!
> Steven



--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413