Patchwork [DF] do not call df_insn_delete in remove_insn, only unlink the insn

login
register
mail settings
Submitter Steven Bosscher
Date April 13, 2013, 12:02 a.m.
Message ID <CABu31nPOrUS_W67+5P2vDO769u0yt2fQgytymXOFLzoFa=3fxQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/236284/
State New
Headers show

Comments

Steven Bosscher - April 13, 2013, 12:02 a.m.
Hello,

emit-rtl.c:remove_insn calls df_insn_delete on insns that have not
been emitted to the insn chain, and therefore don't have DF cache
entries yet. I think it's wrong for remove_insn to call
df_insn_delete.

delete_insn should be used to completely destroy an insn and all
associated data including DF caches. Calling remove_insn to really
delete an insn is also wrong because LABEL_NUSES isn't updated.
Fortunately, almost all code already uses delete_insn so this patch is
small.

remove_insn should only unlink an insn from the current insns chain
(the function body or an open start_sequence chain).

The attached patch makes it so... In fact, without this patch,
sel-sched-ir.c had to work around this problem (remove_insn doing
df_insn_delete) but it was leaking DF caches as a result. Probably
there are other places that can be simplified now to use remove_insn
instead of hacking the insn chain by hand. I've looked at all
remove_insn and delete_insn callers to make sure this patch does

In general I think it's wrong for emit-rtl to have any dependence on
DF (DF depends on RTL, but not the other way around) but that's not
something I want to work on right now.

Bootstrapped&tested on x86_64-unknown-linux-gnu. OK?

Ciao!
Steven
* emit-rtl.c (remove_insn): Do not call df_insn_delete here.
	* cfgrtl.c (delete_insn): Call it here instead.
	* lra-spills.c (lra_final_code_change): Use delete_insn.
	* haifa-sched.c (sched_remove_insn): Likewise.
	* sel-sched-ir.c (return_nop_to_pool): Clear INSN_DELETED_P for nops
	returning to the nop pool.
	(sel_remove_insn): Simplify the only_disconnect case via remove_insn,
	use delete_insn for definitive removal.  Clear BLOCK_FOR_INSN.
Paolo Bonzini - April 13, 2013, 7:14 a.m.
Il 13/04/2013 02:02, Steven Bosscher ha scritto:
> 
> 	* emit-rtl.c (remove_insn): Do not call df_insn_delete here.
> 	* cfgrtl.c (delete_insn): Call it here instead.
> 	* lra-spills.c (lra_final_code_change): Use delete_insn.
> 	* haifa-sched.c (sched_remove_insn): Likewise.
> 	* sel-sched-ir.c (return_nop_to_pool): Clear INSN_DELETED_P for nops
> 	returning to the nop pool.
> 	(sel_remove_insn): Simplify the only_disconnect case via remove_insn,
> 	use delete_insn for definitive removal.  Clear BLOCK_FOR_INSN.

Ok, thanks!

Paolo

Patch

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 197536)
+++ emit-rtl.c	(working copy)
@@ -3908,8 +3908,21 @@  set_insn_deleted (rtx insn)
 }
 
 
-/* Remove an insn from its doubly-linked list.  This function knows how
-   to handle sequences.  */
+/* Unlink INSN from the insn chain.
+
+   This function knows how to handle sequences.
+   
+   This function does not invalidate data flow information associated with
+   INSN (i.e. does not call df_insn_delete).  That makes this function
+   usable for only disconnecting an insn from the chain, and re-emit it
+   elsewhere later.
+
+   To later insert INSN elsewhere in the insn chain via add_insn and
+   similar functions, PREV_INSN and NEXT_INSN must be nullified by
+   the caller.  Nullifying them here breaks many insn chain walks.
+
+   To really delete an insn and related DF information, use delete_insn.  */
+
 void
 remove_insn (rtx insn)
 {
@@ -3968,10 +3981,6 @@  remove_insn (rtx insn)
       gcc_assert (stack);
     }
 
-  /* Invalidate data flow information associated with INSN.  */
-  if (INSN_P (insn))
-    df_insn_delete (insn);
-
   /* Fix up basic block boundaries, if necessary.  */
   if (!BARRIER_P (insn)
       && (bb = BLOCK_FOR_INSN (insn)))
Index: cfgrtl.c
===================================================================
--- cfgrtl.c	(revision 197536)
+++ cfgrtl.c	(working copy)
@@ -164,6 +164,8 @@  delete_insn (rtx insn)
     {
       /* If this insn has already been deleted, something is very wrong.  */
       gcc_assert (!INSN_DELETED_P (insn));
+      if (INSN_P (insn))
+	df_insn_delete (insn);
       remove_insn (insn);
       INSN_DELETED_P (insn) = 1;
     }
Index: lra-spills.c
===================================================================
--- lra-spills.c	(revision 197536)
+++ lra-spills.c	(working copy)
@@ -639,7 +639,7 @@  lra_final_code_change (void)
 		 need them anymore and don't want to waste compiler
 		 time processing them in a few subsequent passes.  */
 	      lra_invalidate_insn_data (insn);
-	      remove_insn (insn);
+	      delete_insn (insn);
 	      continue;
 	    }
 
Index: haifa-sched.c
===================================================================
--- haifa-sched.c	(revision 197536)
+++ haifa-sched.c	(working copy)
@@ -8198,7 +8198,7 @@  sched_remove_insn (rtx insn)
 
   change_queue_index (insn, QUEUE_NOWHERE);
   current_sched_info->add_remove_insn (insn, 1);
-  remove_insn (insn);
+  delete_insn (insn);
 }
 
 /* Clear priorities of all instructions, that are forward dependent on INSN.
Index: sel-sched-ir.c
===================================================================
--- sel-sched-ir.c	(revision 197536)
+++ sel-sched-ir.c	(working copy)
@@ -1065,6 +1065,9 @@  return_nop_to_pool (insn_t nop, bool ful
   gcc_assert (INSN_IN_STREAM_P (nop));
   sel_remove_insn (nop, false, full_tidying);
 
+  /* We'll recycle this nop.  */
+  INSN_DELETED_P (nop) = 0;
+
   if (nop_pool.n == nop_pool.s)
     nop_pool.v = XRESIZEVEC (rtx, nop_pool.v,
                              (nop_pool.s = 2 * nop_pool.s + 1));
@@ -3929,31 +3932,19 @@  sel_remove_insn (insn_t insn, bool only_
     }
 
   if (only_disconnect)
-    {
-      insn_t prev = PREV_INSN (insn);
-      insn_t next = NEXT_INSN (insn);
-      basic_block bb = BLOCK_FOR_INSN (insn);
-
-      NEXT_INSN (prev) = next;
-      PREV_INSN (next) = prev;
-
-      if (BB_HEAD (bb) == insn)
-        {
-          gcc_assert (BLOCK_FOR_INSN (prev) == bb);
-          BB_HEAD (bb) = prev;
-        }
-      if (BB_END (bb) == insn)
-        BB_END (bb) = prev;
-    }
+    remove_insn (insn);
   else
     {
-      remove_insn (insn);
+      delete_insn (insn);
       clear_expr (INSN_EXPR (insn));
     }
 
-  /* It is necessary to null this fields before calling add_insn ().  */
+  /* It is necessary to NULL these fields in case we are going to re-insert
+     INSN into the insns stream, as will usually happen in the ONLY_DISCONNECT
+     case, but also for NOPs that we will return to the nop pool.  */
   PREV_INSN (insn) = NULL_RTX;
   NEXT_INSN (insn) = NULL_RTX;
+  set_block_for_insn (insn, NULL);
 
   return tidy_control_flow (bb, full_tidying);
 }