Patchwork Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047)

login
register
mail settings
Submitter Teresa Johnson
Date Oct. 30, 2012, 5:20 a.m.
Message ID <20121030052000.A4D0F61459@tjsboxrox.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/195288/
State New
Headers show

Comments

Teresa Johnson - Oct. 30, 2012, 5:20 a.m.
This patch fixes three different failures I encountered while trying to use
-freorder-blocks-and-partition, including the failure reported in PR 53743.

Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk?

Thanks,
Teresa

2012-10-29  Teresa Johnson  <tejohnson@google.com>

	PR optimization/53743
        * function.c (thread_prologue_and_epilogue_insns): Don't
        store exit predecessor BB until after it is potentially split.
	* bb-reorder.c (insert_section_boundary_note): Ensure that
        a barrier exists before a switch section node, as this is expected
        by later passes (e.g. dwarf CFI code).
	* cfgrtl.c (rtl_can_merge_blocks): Use the same condition looking
        for region-crossing jumps as in try_redirect_by_replacing_jump,
        which may be called while merging blocks.
	(cfg_layout_can_merge_blocks_p): Ditto.


--
This patch is available for review at http://codereview.appspot.com/6823047
Matthew Gretton-Dann - Oct. 30, 2012, 7:49 a.m.
On 30 October 2012 05:20, Teresa Johnson <tejohnson@google.com> wrote:
> 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;
>
>    /* Protect the loop latches.  */
> @@ -3978,7 +3979,8 @@ cfg_layout_can_merge_blocks_p (basic_block a, basi
>       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;
>
>    /* Protect the loop latches.  */

As this if() condition seems to be the canonical way to detect being
in a different partition should it be moved out into a query function,
and all of cfgrtl.c updated to use it?

[Note I am not a maintainer and so can't approve/reject your patch].

Thanks,

Matt
Steven Bosscher - Oct. 30, 2012, 4:27 p.m.
On Tue, Oct 30, 2012 at 8:49 AM, Matthew Gretton-Dann wrote:
> On 30 October 2012 05:20, Teresa Johnson wrote:
>> 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;
>>
>>    /* Protect the loop latches.  */
>> @@ -3978,7 +3979,8 @@ cfg_layout_can_merge_blocks_p (basic_block a, basi
>>       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;
>>
>>    /* Protect the loop latches.  */
>
> As this if() condition seems to be the canonical way to detect being
> in a different partition should it be moved out into a query function,
> and all of cfgrtl.c updated to use it?

Not just in cfgrtl.c but for example also in ifcvt.c (which currently
only tests for notes, that's broken).

Ciao!
Steven

Patch

Index: function.c
===================================================================
--- function.c	(revision 192692)
+++ function.c	(working copy)
@@ -6517,7 +6517,7 @@  epilogue_done:
       basic_block simple_return_block_cold = NULL;
       edge pending_edge_hot = NULL;
       edge pending_edge_cold = NULL;
-      basic_block exit_pred = EXIT_BLOCK_PTR->prev_bb;
+      basic_block exit_pred;
       int i;
 
       gcc_assert (entry_edge != orig_entry_edge);
@@ -6545,6 +6545,12 @@  epilogue_done:
 	    else
 	      pending_edge_cold = e;
 	  }
+      
+      /* Save a pointer to the exit's predecessor BB for use in
+         inserting new BBs at the end of the function. Do this
+         after the call to split_block above which may split
+         the original exit pred.  */
+      exit_pred = EXIT_BLOCK_PTR->prev_bb;
 
       FOR_EACH_VEC_ELT (edge, unconverted_simple_returns, i, e)
 	{
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));
 	  new_note = emit_note_before (NOTE_INSN_SWITCH_TEXT_SECTIONS,
 				       BB_HEAD (bb));
 	  /* ??? This kind of note always lives between basic blocks,
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;
 
   /* Protect the loop latches.  */
@@ -3978,7 +3979,8 @@  cfg_layout_can_merge_blocks_p (basic_block a, basi
      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;
 
   /* Protect the loop latches.  */