diff mbox

Make max_issue honor issue_rate

Message ID 4CC1D982.7070407@codesourcery.com
State New
Headers show

Commit Message

Jie Zhang Oct. 22, 2010, 6:35 p.m. UTC 
On 10/21/2010 12:41 AM, Jie Zhang wrote:
> Hi,
>
> This patch is an improvement of the patch I posted before:
>
> http://gcc.gnu.org/ml/gcc/2010-10/msg00278.html
>
> According to the replies to that email, it seems both dropping
> ISSUE_POINTS and making max_issue honor issue_rate are good. So I clean
> it up and resubmit it here for review and approval.
>
> This new patch does not enable "gcc_assert (more_issue >= 0);" since
> it's belongs to another issue and I have sent a patch for that.
>
> I also added a new gcc_assert:
>
> gcc_assert (memcmp (top->state, state, dfa_state_size) != 0);
>
> This is mainly for testing purpose. I will remove it if the test results
> are good.
>
> It's already bootstrapped on x86_64-unknown-linux-gnu. The regression
> testing is going on. Is it OK if no regressions are found?
>
The regression testing is OK on x86_64-unknown-linux-gnu. The updated 
patch is attached. OK?

Comments

Jie Zhang Oct. 25, 2010, 6:36 p.m. UTC | #1 
Maxim,

Thanks for review!

On 10/26/2010 02:13 AM, Maxim Kuvyrkov wrote:
> On 10/22/10 2:35 PM, Jie Zhang wrote:
>> (struct choice_entry): Remove field n.
>
> I believe the choice_entry->n field is still useful. It can be used to
> handle incomplete pipeline descriptions for which
> gcc_assert (memcmp (top->state, state, dfa_state_size) != 0);
> would be false. Or to avoid counting pseudo instructions that don't
> affect CPU state.
>
Yeah, that part is what I'm not very sure. My understanding of issue 
rate is that that's the maximum number of instructions we can issue in 
one cycle. Even the instructions does not change the state, it's still 
an instruction. So it should still be under control of issue rate. For 
example, assume we have

4 units: a, b, c, d.

insn 1 uses a, insn 2 uses b, insn 3 uses c, insn 4 uses d.

and we set issue_rate to 3. In this case, we can only issue insn 1, 2, 3 
but not 4 although its unit is free.

To extend this example a little, i.e. insn4 uses nothing, (it does not 
change the state, right? I'm not very sure), it still can not be issued.

So I think we should use "top - choice_stack >= more_issue" instead of 
"top->n >= more_issue". Am I right?


Regards,
Maxim Kuvyrkov Oct. 25, 2010, 9:58 p.m. UTC | #2 
On 10/25/10 2:36 PM, Jie Zhang wrote:

> 4 units: a, b, c, d.
>
> insn 1 uses a, insn 2 uses b, insn 3 uses c, insn 4 uses d.
>
> and we set issue_rate to 3. In this case, we can only issue insn 1, 2, 3
> but not 4 although its unit is free.
>
> To extend this example a little, i.e. insn4 uses nothing, (it does not
> change the state, right? I'm not very sure), it still can not be issued.
>
> So I think we should use "top - choice_stack >= more_issue" instead of
> "top->n >= more_issue". Am I right?

I think the 'n' field was introduced for a reason.  DFA description has 
the "nothing" reservation that specifies that a particular instruction 
does not have a noticeable effect on a CPU (imagine a nop instruction 
that gets optimized out by decoder and does not get into the pipeline).

Regards,
diff mbox

Patch


	* haifa-sched.c (ISSUE_POINTS): Remove.
	(struct choice_entry): Remove field n.
	(max_issue): Don't issue more than issue_rate instructions.

Index: haifa-sched.c
===================================================================
--- haifa-sched.c	(revision 165835)
+++ haifa-sched.c	(working copy)
@@ -199,10 +199,6 @@  struct common_sched_info_def *common_sch
 /* The minimal value of the INSN_TICK of an instruction.  */
 #define MIN_TICK (-max_insn_queue_index)
 
-/* Issue points are used to distinguish between instructions in max_issue ().
-   For now, all instructions are equally good.  */
-#define ISSUE_POINTS(INSN) 1
-
 /* List of important notes we must keep around.  This is a pointer to the
    last element in the list.  */
 rtx note_list;
@@ -2401,8 +2397,6 @@  struct choice_entry
   int index;
   /* The number of the rest insns whose issues we should try.  */
   int rest;
-  /* The number of issued essential insns.  */
-  int n;
   /* State after issuing the insn.  */
   state_t state;
 };
@@ -2444,8 +2438,7 @@  static int cached_issue_rate = 0;
    insns is insns with the best rank (the first insn in READY).  To
    make this function tries different samples of ready insns.  READY
    is current queue `ready'.  Global array READY_TRY reflects what
-   insns are already issued in this try.  MAX_POINTS is the sum of points
-   of all instructions in READY.  The function stops immediately,
+   insns are already issued in this try.  The function stops immediately,
    if it reached the such a solution, that all instruction can be issued.
    INDEX will contain index of the best insn in READY.  The following
    function is used only for first cycle multipass scheduling.
@@ -2458,7 +2451,7 @@  int
 max_issue (struct ready_list *ready, int privileged_n, state_t state,
 	   int *index)
 {
-  int n, i, all, n_ready, best, delay, tries_num, max_points;
+  int i, all, n_ready, best, delay, tries_num;
   int more_issue;
   struct choice_entry *top;
   rtx insn;
@@ -2477,7 +2470,6 @@  max_issue (struct ready_list *ready, int
     }
 
   /* Init max_points.  */
-  max_points = 0;
   more_issue = issue_rate - cycle_issued_insns;
 
   /* ??? We used to assert here that we never issue more insns than issue_rate.
@@ -2488,15 +2480,6 @@  max_issue (struct ready_list *ready, int
 
      gcc_assert (more_issue >= 0);  */
 
-  for (i = 0; i < n_ready; i++)
-    if (!ready_try [i])
-      {
-	if (more_issue-- > 0)
-	  max_points += ISSUE_POINTS (ready_element (ready, i));
-	else
-	  break;
-      }
-
   /* The number of the issued insns in the best solution.  */
   best = 0;
 
@@ -2505,7 +2488,6 @@  max_issue (struct ready_list *ready, int
   /* Set initial state of the search.  */
   memcpy (top->state, state, dfa_state_size);
   top->rest = dfa_lookahead;
-  top->n = 0;
 
   /* Count the number of the insns to search among.  */
   for (all = i = 0; i < n_ready; i++)
@@ -2521,16 +2503,23 @@  max_issue (struct ready_list *ready, int
 	     been asked...  */
 	  top->rest == 0
 	  /* Or have nothing else to try.  */
-	  || i >= n_ready)
+	  || i >= n_ready
+	  /* Or can't issue more.  */
+	  || top - choice_stack >= more_issue)
 	{
 	  /* ??? (... || i == n_ready).  */
 	  gcc_assert (i <= n_ready);
 
+	  /* We should not issue more than issue_rate instructions.  */
+	  gcc_assert (top - choice_stack <= more_issue);
+
 	  if (top == choice_stack)
 	    break;
 
 	  if (best < top - choice_stack)
 	    {
+	      int n;
+
 	      if (privileged_n)
 		{
 		  n = privileged_n;
@@ -2548,7 +2537,7 @@  max_issue (struct ready_list *ready, int
 		  /* This is the index of the insn issued first in this
 		     solution.  */
 		  *index = choice_stack [1].index;
-		  if (top->n == max_points || best == all)
+		  if (top - choice_stack == more_issue || best == all)
 		    break;
 		}
 	    }
@@ -2579,16 +2568,11 @@  max_issue (struct ready_list *ready, int
 	      else
 		top->rest--;
 
-	      n = top->n;
-	      if (memcmp (top->state, state, dfa_state_size) != 0)
-		n += ISSUE_POINTS (insn);
-
 	      /* Advance to the next choice_entry.  */
 	      top++;
 	      /* Initialize it.  */
 	      top->rest = dfa_lookahead;
 	      top->index = i;
-	      top->n = n;
 	      memcpy (top->state, state, dfa_state_size);
 
 	      ready_try [i] = 1;