Patchwork [PR,debug/45136] revamp handling of BB boundary debug insns in sched

login
register
mail settings
Submitter Alexandre Oliva
Date Jan. 19, 2011, 10:26 p.m.
Message ID <or1v48fs2f.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/79607/
State New
Headers show

Comments

Alexandre Oliva - Jan. 19, 2011, 10:26 p.m.
Ping^2?

On Nov 30, 2010, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Sep  4, 2010, Alexandre Oliva <aoliva@redhat.com> wrote:

>> We used to mark and skip debug insns at beginning and end of basic
>> blocks during sched initialization, so as to leave them alone along with
>> any notes that might appear between them and non-debug insns, because in
>> a compilation without debug insns, the notes would remain in place for
>> being outside the blocks.

>> This worked pretty well, except when multi-block sched happened to pull
>> an insn from a later block, causing a debug insns (and notes between
>> them and other insns) to become boundary insns.  We wouldn't
>> re-recognize them as boundary insns or notes, so we'd end up moving the
>> notes about, whereas the non-debug compilation would leave them in
>> place, causing -fcompare-debug differences.

>> This patch does away with the notion of boundary debug insns and notes.
>> Instead, whenever we're skip boundary notes and we find debug insns, we
>> search further for notes and move them past the debug insns, so that
>> they're left alone by the scheduler, as they would in a non-debug
>> compilation.

>> There are two catches:

>> - if notes ever become meaningful among debug insns, this will kind of
>> change their meaning.  I suppose we can cross that bridge when we get to
>> it.

>> - if there's a long chain of debug insns at the beginning or end of a
>> block, we'll iterate over all of them looking for notes every time we
>> call get_ebb_head_tail for such blocks.  I'm not sure this could become
>> a performance issue, and I don't quite see how to avoid checking it
>> every time, considering that the status of a debug insn may change from
>> one call to another, without changes to a previously checked boundary
>> debug insn, e.g.:

>> (debug_insn 51 ...
>> (note 52 ...
>> (insn 53 ...
>> (debug_insn 55 ...
>> (note 54 ...
>> end of block

>> say the patch moved note 54 past debug_insn 55, so debug_insn 55 is
>> regarded as the scheduling boundary.  Then, sched moves insn 53 to an
>> earlier block.  Now, we have to move note 52 past debug_insn 55, but if
>> we optimized the search at the end of the block to stop at debug_insn
>> 55, we'd miss the change of status of note 52 and debug_insn 51.

>> Thoughts?

>> This patch was regstrapped on x86_64-linux-gnu and i686-pc-linux-gnu.
>> Ok to install?

> This no longer bootstrapped recently.  The symptom was moving a
> NOTE_INSN_FUNCTION_BEGIN out of block 2, so that the debug dumps would
> no longer match the debug dumps, in which the note was still at the end
> of block 2.

> I couldn't quite figure out why in some cases notes are moved out of
> blocks, and in other cases they aren't, so I ended up just punting at
> it, removing block numbers from all notes before the final dump.

> Better suggestions are welcome.

> This was again regstrapped on x86_64-linux-gnu and i686-pc-linux-gnu.
> Ok to install?
Richard Henderson - Jan. 24, 2011, 6:49 p.m.
On 01/19/2011 02:26 PM, Alexandre Oliva wrote:
> 	PR debug/45136
> 	PR debug/45130
> 	* haifa-sched.c (get_ebb_head_tail): Move notes across boundary
> 	debug insns.
> 	(no_real_insns_p, schedule_block, set_priorities): Drop special
> 	treatment of boundary debug insns.
> 	* sched-deps.c (sd_init_insn, sd_finish_insn): Don't mark debug
> 	insns.
> 	* sched-ebb.c (schedule_ebbs): Don't skip debug insns.
> 	* sched-int.h (DEBUG_INSN_SCHED_P): Remove.
> 	(BOUNDARY_DEBUG_INSN_P): Likewise.
> 	(SCHEDULE_DEBUG_INSN_P): Likewise.
> 	* sched-rgn.c (init_ready_list): Drop special treatment of
> 	boundary debug insns.
> 	* final.c (rest_of_clean-state): Clear notes' BB.

Ok.


r~
H.J. Lu - Jan. 27, 2011, 11:15 p.m.
On Mon, Jan 24, 2011 at 10:49 AM, Richard Henderson <rth@redhat.com> wrote:
> On 01/19/2011 02:26 PM, Alexandre Oliva wrote:
>>       PR debug/45136
>>       PR debug/45130
>>       * haifa-sched.c (get_ebb_head_tail): Move notes across boundary
>>       debug insns.
>>       (no_real_insns_p, schedule_block, set_priorities): Drop special
>>       treatment of boundary debug insns.
>>       * sched-deps.c (sd_init_insn, sd_finish_insn): Don't mark debug
>>       insns.
>>       * sched-ebb.c (schedule_ebbs): Don't skip debug insns.
>>       * sched-int.h (DEBUG_INSN_SCHED_P): Remove.
>>       (BOUNDARY_DEBUG_INSN_P): Likewise.
>>       (SCHEDULE_DEBUG_INSN_P): Likewise.
>>       * sched-rgn.c (init_ready_list): Drop special treatment of
>>       boundary debug insns.
>>       * final.c (rest_of_clean-state): Clear notes' BB.
>
> Ok.

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47498
H.J. Lu - Jan. 28, 2011, 3:23 a.m.
On Thu, Jan 27, 2011 at 3:15 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jan 24, 2011 at 10:49 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 01/19/2011 02:26 PM, Alexandre Oliva wrote:
>>>       PR debug/45136
>>>       PR debug/45130
>>>       * haifa-sched.c (get_ebb_head_tail): Move notes across boundary
>>>       debug insns.
>>>       (no_real_insns_p, schedule_block, set_priorities): Drop special
>>>       treatment of boundary debug insns.
>>>       * sched-deps.c (sd_init_insn, sd_finish_insn): Don't mark debug
>>>       insns.
>>>       * sched-ebb.c (schedule_ebbs): Don't skip debug insns.
>>>       * sched-int.h (DEBUG_INSN_SCHED_P): Remove.
>>>       (BOUNDARY_DEBUG_INSN_P): Likewise.
>>>       (SCHEDULE_DEBUG_INSN_P): Likewise.
>>>       * sched-rgn.c (init_ready_list): Drop special treatment of
>>>       boundary debug insns.
>>>       * final.c (rest_of_clean-state): Clear notes' BB.
>>
>> Ok.
>
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47498
>

This also caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47501

Patch

for gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/45136
	PR debug/45130
	* haifa-sched.c (get_ebb_head_tail): Move notes across boundary
	debug insns.
	(no_real_insns_p, schedule_block, set_priorities): Drop special
	treatment of boundary debug insns.
	* sched-deps.c (sd_init_insn, sd_finish_insn): Don't mark debug
	insns.
	* sched-ebb.c (schedule_ebbs): Don't skip debug insns.
	* sched-int.h (DEBUG_INSN_SCHED_P): Remove.
	(BOUNDARY_DEBUG_INSN_P): Likewise.
	(SCHEDULE_DEBUG_INSN_P): Likewise.
	* sched-rgn.c (init_ready_list): Drop special treatment of
	boundary debug insns.
	* final.c (rest_of_clean-state): Clear notes' BB.

Index: gcc/haifa-sched.c
===================================================================
--- gcc/haifa-sched.c.orig	2010-11-26 05:37:55.143932457 -0200
+++ gcc/haifa-sched.c	2010-11-26 18:10:21.300655351 -0200
@@ -1901,8 +1901,30 @@  get_ebb_head_tail (basic_block beg, basi
     beg_head = NEXT_INSN (beg_head);
 
   while (beg_head != beg_tail)
-    if (NOTE_P (beg_head) || BOUNDARY_DEBUG_INSN_P (beg_head))
+    if (NOTE_P (beg_head))
       beg_head = NEXT_INSN (beg_head);
+    else if (DEBUG_INSN_P (beg_head))
+      {
+	rtx note, next;
+
+	for (note = NEXT_INSN (beg_head);
+	     note != beg_tail;
+	     note = next)
+	  {
+	    next = NEXT_INSN (note);
+	    if (NOTE_P (note))
+	      {
+		if (sched_verbose >= 9)
+		  fprintf (sched_dump, "reorder %i\n", INSN_UID (note));
+
+		reorder_insns_nobb (note, note, PREV_INSN (beg_head));
+	      }
+	    else if (!DEBUG_INSN_P (note))
+	      break;
+	  }
+
+	break;
+      }
     else
       break;
 
@@ -1914,8 +1936,33 @@  get_ebb_head_tail (basic_block beg, basi
     end_head = NEXT_INSN (end_head);
 
   while (end_head != end_tail)
-    if (NOTE_P (end_tail) || BOUNDARY_DEBUG_INSN_P (end_tail))
+    if (NOTE_P (end_tail))
       end_tail = PREV_INSN (end_tail);
+    else if (DEBUG_INSN_P (end_tail))
+      {
+	rtx note, prev;
+
+	for (note = PREV_INSN (end_tail);
+	     note != end_head;
+	     note = prev)
+	  {
+	    prev = PREV_INSN (note);
+	    if (NOTE_P (note))
+	      {
+		if (sched_verbose >= 9)
+		  fprintf (sched_dump, "reorder %i\n", INSN_UID (note));
+
+		reorder_insns_nobb (note, note, end_tail);
+
+		if (end_tail == BB_END (end))
+		  df_insn_change_bb (note, NULL);
+	      }
+	    else if (!DEBUG_INSN_P (note))
+	      break;
+	  }
+
+	break;
+      }
     else
       break;
 
@@ -1929,8 +1976,7 @@  no_real_insns_p (const_rtx head, const_r
 {
   while (head != NEXT_INSN (tail))
     {
-      if (!NOTE_P (head) && !LABEL_P (head)
-	  && !BOUNDARY_DEBUG_INSN_P (head))
+      if (!NOTE_P (head) && !LABEL_P (head))
 	return 0;
       head = NEXT_INSN (head);
     }
@@ -2813,7 +2859,7 @@  schedule_block (basic_block *target_bb)
   last_scheduled_insn = prev_head;
 
   gcc_assert ((NOTE_P (last_scheduled_insn)
-	       || BOUNDARY_DEBUG_INSN_P (last_scheduled_insn))
+	       || DEBUG_INSN_P (last_scheduled_insn))
 	      && BLOCK_FOR_INSN (last_scheduled_insn) == *target_bb);
 
   /* Initialize INSN_QUEUE.  Q_SIZE is the total number of insns in the
@@ -3320,7 +3366,7 @@  set_priorities (rtx head, rtx tail)
 	current_sched_info->sched_max_insns_priority;
   rtx prev_head;
 
-  if (head == tail && (! INSN_P (head) || BOUNDARY_DEBUG_INSN_P (head)))
+  if (head == tail && ! INSN_P (head))
     gcc_unreachable ();
 
   n_insn = 0;
Index: gcc/sched-deps.c
===================================================================
--- gcc/sched-deps.c.orig	2010-11-26 05:37:55.142932376 -0200
+++ gcc/sched-deps.c	2010-11-26 16:27:26.110757202 -0200
@@ -717,9 +717,6 @@  sd_init_insn (rtx insn)
   INSN_FORW_DEPS (insn) = create_deps_list ();
   INSN_RESOLVED_FORW_DEPS (insn) = create_deps_list ();
 
-  if (DEBUG_INSN_P (insn))
-    DEBUG_INSN_SCHED_P (insn) = TRUE;
-
   /* ??? It would be nice to allocate dependency caches here.  */
 }
 
@@ -729,12 +726,6 @@  sd_finish_insn (rtx insn)
 {
   /* ??? It would be nice to deallocate dependency caches here.  */
 
-  if (DEBUG_INSN_P (insn))
-    {
-      gcc_assert (DEBUG_INSN_SCHED_P (insn));
-      DEBUG_INSN_SCHED_P (insn) = FALSE;
-    }
-
   free_deps_list (INSN_HARD_BACK_DEPS (insn));
   INSN_HARD_BACK_DEPS (insn) = NULL;
 
Index: gcc/sched-ebb.c
===================================================================
--- gcc/sched-ebb.c.orig	2010-11-26 05:37:55.142932376 -0200
+++ gcc/sched-ebb.c	2010-11-26 16:27:26.198764286 -0200
@@ -600,9 +600,9 @@  schedule_ebbs (void)
 	 a note or two.  */
       while (head != tail)
 	{
-	  if (NOTE_P (head) || BOUNDARY_DEBUG_INSN_P (head))
+	  if (NOTE_P (head))
 	    head = NEXT_INSN (head);
-	  else if (NOTE_P (tail) || BOUNDARY_DEBUG_INSN_P (tail))
+	  else if (NOTE_P (tail))
 	    tail = PREV_INSN (tail);
 	  else if (LABEL_P (head))
 	    head = NEXT_INSN (head);
Index: gcc/sched-int.h
===================================================================
--- gcc/sched-int.h.orig	2010-11-26 05:37:55.143932457 -0200
+++ gcc/sched-int.h	2010-11-26 16:27:26.267769840 -0200
@@ -887,23 +887,6 @@  extern VEC(haifa_deps_insn_data_def, hea
 #define IS_SPECULATION_BRANCHY_CHECK_P(INSN) \
   (RECOVERY_BLOCK (INSN) != NULL && RECOVERY_BLOCK (INSN) != EXIT_BLOCK_PTR)
 
-/* The unchanging bit tracks whether a debug insn is to be handled
-   like an insn (i.e., schedule it) or like a note (e.g., it is next
-   to a basic block boundary.  */
-#define DEBUG_INSN_SCHED_P(insn) \
-  (RTL_FLAG_CHECK1("DEBUG_INSN_SCHED_P", (insn), DEBUG_INSN)->unchanging)
-
-/* True if INSN is a debug insn that is next to a basic block
-   boundary, i.e., it is to be handled by the scheduler like a
-   note.  */
-#define BOUNDARY_DEBUG_INSN_P(insn) \
-  (DEBUG_INSN_P (insn) && !DEBUG_INSN_SCHED_P (insn))
-/* True if INSN is a debug insn that is not next to a basic block
-   boundary, i.e., it is to be handled by the scheduler like an
-   insn.  */
-#define SCHEDULE_DEBUG_INSN_P(insn) \
-  (DEBUG_INSN_P (insn) && DEBUG_INSN_SCHED_P (insn))
-
 /* Dep status (aka ds_t) of the link encapsulates information, that is needed
    for speculative scheduling.  Namely, it is 4 integers in the range
    [0, MAX_DEP_WEAK] and 3 bits.
Index: gcc/sched-rgn.c
===================================================================
--- gcc/sched-rgn.c.orig	2010-11-26 05:37:55.143932457 -0200
+++ gcc/sched-rgn.c	2010-11-26 16:27:26.383779177 -0200
@@ -2140,7 +2140,7 @@  init_ready_list (void)
 	src_head = head;
 
 	for (insn = src_head; insn != src_next_tail; insn = NEXT_INSN (insn))
-	  if (INSN_P (insn) && !BOUNDARY_DEBUG_INSN_P (insn))
+	  if (INSN_P (insn))
 	    try_ready (insn);
       }
 }
Index: gcc/final.c
===================================================================
--- gcc/final.c.orig	2010-11-26 05:37:55.142932376 -0200
+++ gcc/final.c	2010-11-26 16:27:26.729807027 -0200
@@ -4366,7 +4366,11 @@  rest_of_clean_state (void)
 	    if (LABEL_P (insn))
 	      INSN_UID (insn) = CODE_LABEL_NUMBER (insn);
 	    else
-	      INSN_UID (insn) = 0;
+	      {
+		if (NOTE_P (insn))
+		  set_block_for_insn (insn, NULL);
+		INSN_UID (insn) = 0;
+	      }
 	}
     }
 
@@ -4387,7 +4391,6 @@  rest_of_clean_state (void)
 	       && NOTE_KIND (insn) != NOTE_INSN_BLOCK_END
 	       && NOTE_KIND (insn) != NOTE_INSN_CFA_RESTORE_STATE)))
 	print_rtl_single (final_output, insn);
-
     }
 
   if (final_output)