diff mbox

Scheduler cleanups, 1/N

Message ID 4D99DEC7.80104@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt April 4, 2011, 3:07 p.m. UTC 
On 04/02/2011 02:55 AM, H.J. Lu wrote:
> On Thu, Mar 24, 2011 at 6:07 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>> I have a number of patches that will be necessary for a new target. Some
>> of these can be applied now as cleanups, so I'm submit them now.
>>
>> This changes the schedule_block main loop not to move instructions while
>> computing the schedule. Instead, we collect them in a VEC and modify the
>> RTL afterwards. The real motivation for this is to add support for
>> backtracking later.
>>
>> Bootstrapped and tested on i686-linux. No changes in generated code on
>> any of my testcases.
>>
> 
> This caused:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48403

I eventually managed to reproduce it, and even figured out what I'd
stupidly been doing wrong with my bootstraps which caused me not to see
this.

There were a few places where last_scheduled_insn wasn't just examined
on its own, but code wanted to walk backwards and forwards from it. This
patch adapts them. I've also included Steven's patch from the bugzilla.

Bootstrapped on i686-linux. Also tested, but I need to rerun those since
there were other changes in the tree which were causing some failures.
I've also built a powerpc-linux cross compiler, and compiled my set of
examples with "-fsched-stalled-insns-dep=4 -msched-costly-dep=all
-fsched-stalled-insns", with both patches included and with both patches
removed, and found no code generation differences. I also did a run
using "-fdbg-cnt=sched_insn:20", which exposed a preexisting bug in
schedule_block (avoiding a requeue for the first insn, but not placing
it back in the ready list).

Ok after retest?


Bernd
* haifa-sched.c (nonscheduled_insns_begin): New static variable.
	(rank_for_schedule): Use scheduled_insns vector instead of
	last_scheduled_insn.
	(ok_for_early_queue_removal): Likewise.
	(queue_to_ready): Search forward in nonscheduled_insns_begin if
	we have a dbg_cnt.
	(choose_ready): Likewise.
	(commit_schedule): Use VEC_iterate.
	(schedule_block): Initialize nonscheduled_insns_begin.  If we have
	a dbg_cnt, use it and ensure the first insn is in the ready list.
	(haifa_sched_init): Allocate scheduled_insns.
	(sched_extend_ready_list): Don't allocate it; reserve space.
	(haifa_sched_finish): Free it.

Comments

H.J. Lu April 4, 2011, 3:37 p.m. UTC | #1 
On Mon, Apr 4, 2011 at 8:07 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 04/02/2011 02:55 AM, H.J. Lu wrote:
>> On Thu, Mar 24, 2011 at 6:07 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>> I have a number of patches that will be necessary for a new target. Some
>>> of these can be applied now as cleanups, so I'm submit them now.
>>>
>>> This changes the schedule_block main loop not to move instructions while
>>> computing the schedule. Instead, we collect them in a VEC and modify the
>>> RTL afterwards. The real motivation for this is to add support for
>>> backtracking later.
>>>
>>> Bootstrapped and tested on i686-linux. No changes in generated code on
>>> any of my testcases.
>>>
>>
>> This caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48403
>
> I eventually managed to reproduce it, and even figured out what I'd
> stupidly been doing wrong with my bootstraps which caused me not to see
> this.
>
> There were a few places where last_scheduled_insn wasn't just examined
> on its own, but code wanted to walk backwards and forwards from it. This
> patch adapts them. I've also included Steven's patch from the bugzilla.
>
> Bootstrapped on i686-linux. Also tested, but I need to rerun those since
> there were other changes in the tree which were causing some failures.
> I've also built a powerpc-linux cross compiler, and compiled my set of
> examples with "-fsched-stalled-insns-dep=4 -msched-costly-dep=all
> -fsched-stalled-insns", with both patches included and with both patches
> removed, and found no code generation differences. I also did a run
> using "-fdbg-cnt=sched_insn:20", which exposed a preexisting bug in
> schedule_block (avoiding a requeue for the first insn, but not placing
> it back in the ready list).
>
> Ok after retest?
>

Could you please mention PR 48403 in your ChangeLog?

Thanks.
Vladimir Makarov April 4, 2011, 4:34 p.m. UTC | #2 
On 04/04/2011 11:07 AM, Bernd Schmidt wrote:
>
> I eventually managed to reproduce it, and even figured out what I'd
> stupidly been doing wrong with my bootstraps which caused me not to see
> this.
>
> There were a few places where last_scheduled_insn wasn't just examined
> on its own, but code wanted to walk backwards and forwards from it. This
> patch adapts them. I've also included Steven's patch from the bugzilla.
>
> Bootstrapped on i686-linux. Also tested, but I need to rerun those since
> there were other changes in the tree which were causing some failures.
> I've also built a powerpc-linux cross compiler, and compiled my set of
> examples with "-fsched-stalled-insns-dep=4 -msched-costly-dep=all
> -fsched-stalled-insns", with both patches included and with both patches
> removed, and found no code generation differences. I also did a run
> using "-fdbg-cnt=sched_insn:20", which exposed a preexisting bug in
> schedule_block (avoiding a requeue for the first insn, but not placing
> it back in the ready list).
>
> Ok after retest?
>
Ok, thanks.
H.J. Lu April 4, 2011, 4:54 p.m. UTC | #3 
On Mon, Apr 4, 2011 at 9:34 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:
> On 04/04/2011 11:07 AM, Bernd Schmidt wrote:
>>
>> I eventually managed to reproduce it, and even figured out what I'd
>> stupidly been doing wrong with my bootstraps which caused me not to see
>> this.
>>
>> There were a few places where last_scheduled_insn wasn't just examined
>> on its own, but code wanted to walk backwards and forwards from it. This
>> patch adapts them. I've also included Steven's patch from the bugzilla.
>>
>> Bootstrapped on i686-linux. Also tested, but I need to rerun those since
>> there were other changes in the tree which were causing some failures.
>> I've also built a powerpc-linux cross compiler, and compiled my set of
>> examples with "-fsched-stalled-insns-dep=4 -msched-costly-dep=all
>> -fsched-stalled-insns", with both patches included and with both patches
>> removed, and found no code generation differences. I also did a run
>> using "-fdbg-cnt=sched_insn:20", which exposed a preexisting bug in
>> schedule_block (avoiding a requeue for the first insn, but not placing
>> it back in the ready list).
>>
>> Ok after retest?
>>
> Ok, thanks.
>
>

I bootstrapped it on Linux/x86-64 and checked it in.

Thanks.
Bernd Schmidt April 4, 2011, 6:11 p.m. UTC | #4 
On 04/04/2011 06:54 PM, H.J. Lu wrote:
> On Mon, Apr 4, 2011 at 9:34 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:
>> On 04/04/2011 11:07 AM, Bernd Schmidt wrote:
>>> Ok after retest?
>>>
>> Ok, thanks.
>>
>>
> 
> I bootstrapped it on Linux/x86-64 and checked it in.

That was slightly premature, but the tests did now finish successfully.


Bernd
diff mbox

Patch

Index: haifa-sched.c
===================================================================
--- haifa-sched.c	(revision 171845)
+++ haifa-sched.c	(working copy)
@@ -780,12 +780,14 @@  print_curr_reg_pressure (void)
   fprintf (sched_dump, "\n");
 }
 
-/* Pointer to the last instruction scheduled.  Used by rank_for_schedule,
-   so that insns independent of the last scheduled insn will be preferred
-   over dependent instructions.  */
-
+/* Pointer to the last instruction scheduled.  */
 static rtx last_scheduled_insn;
 
+/* Pointer that iterates through the list of unscheduled insns if we
+   have a dbg_cnt enabled.  It always points at an insn prior to the
+   first unscheduled one.  */
+static rtx nonscheduled_insns_begin;
+
 /* Cached cost of the instruction.  Use below function to get cost of the
    insn.  -1 here means that the field is not initialized.  */
 #define INSN_COST(INSN)	(HID (INSN)->cost)
@@ -1239,18 +1241,19 @@  rank_for_schedule (const void *x, const 
 
   if (flag_sched_last_insn_heuristic)
     {
-      last = last_scheduled_insn;
-
-      if (DEBUG_INSN_P (last) && last != current_sched_info->prev_head)
-	do
-	  last = PREV_INSN (last);
-	while (!NONDEBUG_INSN_P (last)
-	       && last != current_sched_info->prev_head);
+      int i = VEC_length (rtx, scheduled_insns);
+      last = NULL_RTX;
+      while (i-- > 0)
+	{
+	  last = VEC_index (rtx, scheduled_insns, i);
+	  if (NONDEBUG_INSN_P (last))
+	    break;
+	}
     }
 
   /* Compare insns based on their relation to the last scheduled
      non-debug insn.  */
-  if (flag_sched_last_insn_heuristic && NONDEBUG_INSN_P (last))
+  if (flag_sched_last_insn_heuristic && last && NONDEBUG_INSN_P (last))
     {
       dep_t dep1;
       dep_t dep2;
@@ -2044,9 +2047,16 @@  queue_to_ready (struct ready_list *ready
   q_ptr = NEXT_Q (q_ptr);
 
   if (dbg_cnt (sched_insn) == false)
-    /* If debug counter is activated do not requeue insn next after
-       last_scheduled_insn.  */
-    skip_insn = next_nonnote_nondebug_insn (last_scheduled_insn);
+    {
+      /* If debug counter is activated do not requeue the first
+	 nonscheduled insn.  */
+      skip_insn = nonscheduled_insns_begin;
+      do
+	{
+	  skip_insn = next_nonnote_nondebug_insn (skip_insn);
+	}
+      while (QUEUE_INDEX (skip_insn) == QUEUE_SCHEDULED);
+    }
   else
     skip_insn = NULL_RTX;
 
@@ -2129,22 +2139,18 @@  queue_to_ready (struct ready_list *ready
 static bool
 ok_for_early_queue_removal (rtx insn)
 {
-  int n_cycles;
-  rtx prev_insn = last_scheduled_insn;
-
   if (targetm.sched.is_costly_dependence)
     {
+      rtx prev_insn;
+      int n_cycles;
+      int i = VEC_length (rtx, scheduled_insns);
       for (n_cycles = flag_sched_stalled_insns_dep; n_cycles; n_cycles--)
 	{
-	  for ( ; prev_insn; prev_insn = PREV_INSN (prev_insn))
+	  while (i-- > 0)
 	    {
 	      int cost;
 
-	      if (prev_insn == current_sched_info->prev_head)
-		{
-		  prev_insn = NULL;
-		  break;
-		}
+	      prev_insn = VEC_index (rtx, scheduled_insns, i);
 
 	      if (!NOTE_P (prev_insn))
 		{
@@ -2166,9 +2172,8 @@  ok_for_early_queue_removal (rtx insn)
 		break;
 	    }
 
-	  if (!prev_insn)
+	  if (i == 0)
 	    break;
-	  prev_insn = PREV_INSN (prev_insn);
 	}
     }
 
@@ -2673,13 +2678,17 @@  choose_ready (struct ready_list *ready, 
 
   if (dbg_cnt (sched_insn) == false)
     {
-      rtx insn;
-
-      insn = next_nonnote_insn (last_scheduled_insn);
+      rtx insn = nonscheduled_insns_begin;
+      do
+	{
+	  insn = next_nonnote_insn (insn);
+	}
+      while (QUEUE_INDEX (insn) == QUEUE_SCHEDULED);
 
       if (QUEUE_INDEX (insn) == QUEUE_READY)
 	/* INSN is in the ready_list.  */
 	{
+	  nonscheduled_insns_begin = insn;
 	  ready_remove_insn (insn);
 	  *insn_ptr = insn;
 	  return 0;
@@ -2826,13 +2835,14 @@  choose_ready (struct ready_list *ready, 
 static void
 commit_schedule (rtx prev_head, rtx tail, basic_block *target_bb)
 {
-  int i;
+  unsigned int i;
+  rtx insn;
 
   last_scheduled_insn = prev_head;
-  for (i = 0; i < (int)VEC_length (rtx, scheduled_insns); i++)
+  for (i = 0;
+       VEC_iterate (rtx, scheduled_insns, i, insn);
+       i++)
     {
-      rtx insn = VEC_index (rtx, scheduled_insns, i);
-
       if (control_flow_insn_p (last_scheduled_insn)
 	  || current_sched_info->advance_target_bb (*target_bb, insn))
 	{
@@ -2956,7 +2966,7 @@  schedule_block (basic_block *target_bb)
     targetm.sched.init (sched_dump, sched_verbose, ready.veclen);
 
   /* We start inserting insns after PREV_HEAD.  */
-  last_scheduled_insn = prev_head;
+  last_scheduled_insn = nonscheduled_insns_begin = prev_head;
 
   gcc_assert ((NOTE_P (last_scheduled_insn)
 	       || DEBUG_INSN_P (last_scheduled_insn))
@@ -3001,12 +3011,12 @@  schedule_block (basic_block *target_bb)
 
       /* Delay all insns past it for 1 cycle.  If debug counter is
 	 activated make an exception for the insn right after
-	 last_scheduled_insn.  */
+	 nonscheduled_insns_begin.  */
       {
 	rtx skip_insn;
 
 	if (dbg_cnt (sched_insn) == false)
-	  skip_insn = next_nonnote_insn (last_scheduled_insn);
+	  skip_insn = next_nonnote_insn (nonscheduled_insns_begin);
 	else
 	  skip_insn = NULL_RTX;
 
@@ -3019,6 +3029,8 @@  schedule_block (basic_block *target_bb)
 	    if (insn != skip_insn)
 	      queue_insn (insn, 1, "list truncated");
 	  }
+	if (skip_insn)
+	  ready_add (&ready, skip_insn, true);
       }
     }
 
@@ -3540,6 +3552,8 @@  haifa_sched_init (void)
   setup_sched_dump ();
   sched_init ();
 
+  scheduled_insns = VEC_alloc (rtx, heap, 0);
+
   if (spec_info != NULL)
     {
       sched_deps_info->use_deps_list = 1;
@@ -3610,6 +3624,8 @@  haifa_sched_finish (void)
                c, nr_be_in_control);
     }
 
+  VEC_free (rtx, heap, scheduled_insns);
+
   /* Finalize h_i_d, dependency caches, and luids for the whole
      function.  Target will be finalized in md_global_finish ().  */
   sched_deps_finish ();
@@ -4008,7 +4024,7 @@  sched_extend_ready_list (int new_sched_r
     {
       i = 0;
       sched_ready_n_insns = 0;
-      scheduled_insns = VEC_alloc (rtx, heap, new_sched_ready_n_insns);
+      VEC_reserve (rtx, heap, scheduled_insns, new_sched_ready_n_insns);
     }
   else
     i = sched_ready_n_insns + 1;