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

Steven Bosscher stevenb.gcc@gmail.com
Mon May 20 22:43:00 GMT 2013


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() ?


> 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.


> 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 ;-)


>         {
> -         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).


> @@ -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.

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

Ciao!
Steven



More information about the Gcc-patches mailing list