diff mbox

Remove dead labels to increase superblock scope

Message ID 4EDAB22E.2080204@mentor.com
State New
Headers show

Commit Message

Tom de Vries Dec. 3, 2011, 11:35 p.m. UTC 
On 02/12/11 11:40, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>> On 27/11/11 23:59, Eric Botcazou wrote:
>>>> No, DELETED_LABEL notes still work just fine. It depends on how you
>>>> remove the label and replace it with a note, and Tom isn't showing
>>>> what he did, so...
>>>
>>> I agree that there is no obvious reason why just calling delete_insn would not 
>>> work, so this should be investigated first.
>>>
>>
>> The reason it didn't work, is because after turning a label into a
>> NOTE_INSN_DELETED_LABEL, one needs to move it to after the NOTE_INSN_BASIC_BLOCK
>> as in cfgcleanup.c:try_optimize_cfg():
>> ...
>> 		  delete_insn_chain (label, label, false);
>> 		  /* If the case label is undeletable, move it after the
>> 		     BASIC_BLOCK note.  */
>> 		  if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
>> 		    {
>> 		      rtx bb_note = NEXT_INSN (BB_HEAD (b));
>>
>> 		      reorder_insns_nobb (label, label, bb_note);
>> 		      BB_HEAD (b) = bb_note;
>> 		      if (BB_END (b) == bb_note)
>> 			BB_END (b) = label;
>> 		    }
>> ...
>>
>> Attached patch factors out this piece of code and reuses it in fixup_reorder_chain.
> 
> But isn't...
> 
>> @@ -2637,15 +2658,7 @@ try_optimize_cfg (int mode)
>>  		  delete_insn_chain (label, label, false);
>>  		  /* If the case label is undeletable, move it after the
>>  		     BASIC_BLOCK note.  */
>> -		  if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
>> -		    {
>> -		      rtx bb_note = NEXT_INSN (BB_HEAD (b));
>> -
>> -		      reorder_insns_nobb (label, label, bb_note);
>> -		      BB_HEAD (b) = bb_note;
>> -		      if (BB_END (b) == bb_note)
>> -			BB_END (b) = label;
>> -		    }
>> +		  fixup_deleted_label (b);
> 
> ...this "delete_insn_chain (label, label, false);" call equivalent
> to "delete_insn (label)"?  

Indeed, it is.

> Splitting the operation in two here and:
> 
>> Index: gcc/cfglayout.c
>> ===================================================================
>> --- gcc/cfglayout.c (revision 181652)
>> +++ gcc/cfglayout.c (working copy)
>> @@ -857,6 +857,12 @@ fixup_reorder_chain (void)
>>  				       (e_taken->src, e_taken->dest));
>>  		  e_taken->flags |= EDGE_FALLTHRU;
>>  		  update_br_prob_note (bb);
>> +		  if (LABEL_NUSES (ret_label) == 0
>> +		      && single_pred_p (e_taken->dest))
>> +		    {
>> +		      delete_insn (ret_label);
>> +		      fixup_deleted_label (e_taken->dest);
>> +		    }
> 
> ...here seems a little odd.
> 
> Richard

OK, factored out delete_label now.

Bootstrapped and reg-tested on x86_64.

Ok for next stage1?

Thanks,
- Tom

2011-12-03  Tom de Vries  <tom@codesourcery.com>

	* cfgcleanup.c (delete_label): New function, factored out of ...
	(try_optimize_cfg): Use deleted_label.
	* basic-block.h (delete_label): Declare.
	* cfglayout.c (fixup_reorder_chain): Delete unused label using
	delete_label.

	* gcc.dg/superblock.c: New test.

Comments

Richard Sandiford Dec. 4, 2011, 12:41 p.m. UTC | #1 
Tom de Vries <Tom_deVries@mentor.com> writes:
> OK, factored out delete_label now.
>
> Bootstrapped and reg-tested on x86_64.
>
> Ok for next stage1?

Looks good codewise.  I'm just a bit worried about the name "delete_label".
"delete_insn (label)" should always do the right thing for a pure deletion;
the point of the new routine is that it also moves instructions.
I'd prefer a name that differentiated it from delete_insn.  E.g.
"hide_label" or "decommission_label", although as you can tell
I'm useless at naming things...

Thanks,
Richard
Eric Botcazou Dec. 4, 2011, 3:21 p.m. UTC | #2 
> Looks good codewise.

Seconded, modulo the file: the function should be in cfgrtl.c instead.

> I'm just a bit worried about the name "delete_label". 
> "delete_insn (label)" should always do the right thing for a pure deletion;
> the point of the new routine is that it also moves instructions.

It only fixes things up though, so that the RTL stream is valid again.  Hence 
the question: why not retrofit it into delete_insn directly?
Michael Matz Dec. 5, 2011, 12:50 p.m. UTC | #3 
Hi,

On Sun, 4 Dec 2011, Eric Botcazou wrote:

> > I'm just a bit worried about the name "delete_label". "delete_insn 
> > (label)" should always do the right thing for a pure deletion; the 
> > point of the new routine is that it also moves instructions.
> 
> It only fixes things up though, so that the RTL stream is valid again.  
> Hence the question: why not retrofit it into delete_insn directly?

Was my first reaction too.  What good is delete_insn(label) if it leaves 
the stream in a state to be fixed up immediately.


Ciao,
Michael.
Tom de Vries Dec. 6, 2011, 1:16 p.m. UTC | #4 
On 05/12/11 13:50, Michael Matz wrote:
> Hi,
> 
> On Sun, 4 Dec 2011, Eric Botcazou wrote:
> 
>>> I'm just a bit worried about the name "delete_label". "delete_insn 
>>> (label)" should always do the right thing for a pure deletion; the 
>>> point of the new routine is that it also moves instructions.
>>
>> It only fixes things up though, so that the RTL stream is valid again.  
>> Hence the question: why not retrofit it into delete_insn directly?
> 
> Was my first reaction too.  What good is delete_insn(label) if it leaves 
> the stream in a state to be fixed up immediately.
> 

delete_insn returns the next insn.

If I have:

  (code_label 33 26 34 4 1 "" [0 uses])

  (note 34 33 50 4 [bb 4] NOTE_INSN_BASIC_BLOCK)

  (note 50 34 51 4 NOTE_INSN_EPILOGUE_BEG)

and delete_insn turns the label into a note and changes order into:

  (note 34 33 50 4 [bb 4] NOTE_INSN_BASIC_BLOCK)

  (note 33 26 34 4 1 "" NOTE_INSN_DELETED_LABEL)

  (note 50 34 51 4 NOTE_INSN_EPILOGUE_BEG)

what should be the 'next' returned by delete_insn?

Thanks,
- Tom

> 
> Ciao,
> Michael.
Michael Matz Dec. 6, 2011, 1:25 p.m. UTC | #5 
Hi,

On Tue, 6 Dec 2011, Tom de Vries wrote:

> what should be the 'next' returned by delete_insn?

There are exactly two calls of delete_insn that take the return value.  
One (delete_insn_and_edges) just uses it to return it itself (and there 
are no calls to delete_insn_and_edges that use the returned value), the 
other (delete_insn_chain) wants to have the original next insn (before 
reordering), even if that means that it can see the insn twice (once as 
label, once as note, the latter would be skipped).

So, return the original next one.  Even better would be to somehow clean 
up the single last use of the return value in delete_insn_chain, and make 
delete_insn return nothing.


Ciao,
Michael.
diff mbox

Patch

Index: gcc/cfgcleanup.c
===================================================================
--- gcc/cfgcleanup.c (revision 181172)
+++ gcc/cfgcleanup.c (working copy)
@@ -2518,6 +2518,39 @@  trivially_empty_bb_p (basic_block bb)
     }
 }
 
+/* Delete the label at the head of BB, and if that results in a DELETED_LABEL
+   note, move it after the BASIC_BLOCK note of BB.  */
+
+void
+delete_label (basic_block bb)
+{
+  rtx label = BB_HEAD (bb), deleted_label, bb_note;
+  gcc_assert (LABEL_P (label));
+
+  /* Delete the label.  */
+  delete_insn (label);
+  if (dump_file)
+    fprintf (dump_file, "Deleted label in block %i.\n",
+	     bb->index);
+
+  /* Find the DELETED_LABEL note.  */
+  deleted_label = BB_HEAD (bb);
+  if (deleted_label == NULL_RTX
+      || !NOTE_P (deleted_label)
+      || NOTE_KIND (deleted_label) != NOTE_INSN_DELETED_LABEL)
+    return;
+
+  /* Find the BASIC_BLOCK note.  */
+  bb_note = NEXT_INSN (deleted_label);
+  gcc_assert (bb_note != NULL_RTX && NOTE_INSN_BASIC_BLOCK_P (bb_note));
+
+  /* Move the DELETED_LABEL note after the BASIC_BLOCK note.  */
+  reorder_insns_nobb (deleted_label, deleted_label, bb_note);
+  BB_HEAD (bb) = bb_note;
+  if (BB_END (bb) == bb_note)
+    BB_END (bb) = deleted_label;
+}
+
 /* Do simple CFG optimizations - basic block merging, simplifying of jump
    instructions etc.  Return nonzero if changes were made.  */
 
@@ -2631,25 +2664,7 @@  try_optimize_cfg (int mode)
 		      || !JUMP_P (BB_END (single_pred (b)))
 		      || ! label_is_jump_target_p (BB_HEAD (b),
 						   BB_END (single_pred (b)))))
-		{
-		  rtx label = BB_HEAD (b);
-
-		  delete_insn_chain (label, label, false);
-		  /* If the case label is undeletable, move it after the
-		     BASIC_BLOCK note.  */
-		  if (NOTE_KIND (BB_HEAD (b)) == NOTE_INSN_DELETED_LABEL)
-		    {
-		      rtx bb_note = NEXT_INSN (BB_HEAD (b));
-
-		      reorder_insns_nobb (label, label, bb_note);
-		      BB_HEAD (b) = bb_note;
-		      if (BB_END (b) == bb_note)
-			BB_END (b) = label;
-		    }
-		  if (dump_file)
-		    fprintf (dump_file, "Deleted label in block %i.\n",
-			     b->index);
-		}
+		delete_label (b);
 
 	      /* If we fall through an empty block, we can remove it.  */
 	      if (!(mode & CLEANUP_CFGLAYOUT)
Index: gcc/cfglayout.c
===================================================================
--- gcc/cfglayout.c (revision 181172)
+++ gcc/cfglayout.c (working copy)
@@ -857,6 +857,9 @@  fixup_reorder_chain (void)
 				       (e_taken->src, e_taken->dest));
 		  e_taken->flags |= EDGE_FALLTHRU;
 		  update_br_prob_note (bb);
+		  if (LABEL_NUSES (ret_label) == 0
+		      && single_pred_p (e_taken->dest))
+		    delete_label (e_taken->dest);
 		  continue;
 		}
 	    }
Index: gcc/basic-block.h
===================================================================
--- gcc/basic-block.h (revision 181172)
+++ gcc/basic-block.h (working copy)
@@ -813,6 +813,7 @@  extern void rtl_make_eh_edge (sbitmap, b
 enum replace_direction { dir_none, dir_forward, dir_backward, dir_both };
 
 /* In cfgcleanup.c.  */
+extern void delete_label (basic_block);
 extern bool cleanup_cfg (int);
 extern int flow_find_cross_jump (basic_block, basic_block, rtx *, rtx *,
                                  enum replace_direction*);
Index: gcc/testsuite/gcc.dg/superblock.c
===================================================================
--- /dev/null (new file)
+++ gcc/testsuite/gcc.dg/superblock.c (revision 0)
@@ -0,0 +1,23 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-asynchronous-unwind-tables -fsched2-use-superblocks -fdump-rtl-sched2 -fdump-rtl-bbro" } */
+
+typedef int aligned __attribute__ ((aligned (64)));
+extern void abort (void);
+
+int bar (void *p);
+
+void
+foo (void)
+{
+  char *p = __builtin_alloca (13);
+  aligned i;
+
+  if (bar (p) || bar (&i))
+    abort ();
+}
+
+/* { dg-final { scan-rtl-dump-times "0 uses" 0 "bbro"} } */
+/* { dg-final { scan-rtl-dump-times "ADVANCING TO" 2 "sched2"} } */
+/* { dg-final { cleanup-rtl-dump "bbro" } } */
+/* { dg-final { cleanup-rtl-dump "sched2" } } */
+