diff mbox

[mips] Return correct value from TARGET_SCHED_REORDER2 (PR rtl-optimization/37360)

Message ID 4CBF02B4.4010207@codesourcery.com
State New
Headers show

Commit Message

Jie Zhang Oct. 20, 2010, 2:54 p.m. UTC 
Hi,

Currently TARGET_SCHED_REORDER2 returns mips_issue_rate (), which will 
reset can_issue_more to the issue_rate after scheduling each 
instruction. See haifa-sched.c:sched_block for the details. Because of 
this, issue_rate is not honored when scheduling. This caused PR 
rtl-optimization/37360 since the issue_rate is set to a value smaller 
than the number of instructions that scheduler DFA can issue in one 
cycle for mips sb1.

This patch makes mips target follow rs6000 and sh. It caches 
can_issue_more in TARGET_SCHED_VARIABLE_ISSUE and return the cached 
value in TARGET_SCHED_REORDER2. It also addes the test case from that PR 
and revert the the original hack.

Is it OK?


Regards,

Comments

Richard Sandiford Oct. 20, 2010, 8:44 p.m. UTC | #1 
Jie Zhang <jie@codesourcery.com> writes:
> Currently TARGET_SCHED_REORDER2 returns mips_issue_rate (), which will 
> reset can_issue_more to the issue_rate after scheduling each 
> instruction. See haifa-sched.c:sched_block for the details. Because of 
> this, issue_rate is not honored when scheduling. This caused PR 
> rtl-optimization/37360 since the issue_rate is set to a value smaller 
> than the number of instructions that scheduler DFA can issue in one 
> cycle for mips sb1.

The haifa-sched.c part looks good to me, but I can't approve it.
Thanks for tracking down the underlying problem.

> This patch makes mips target follow rs6000 and sh. It caches 
> can_issue_more in TARGET_SCHED_VARIABLE_ISSUE and return the cached 
> value in TARGET_SCHED_REORDER2. It also addes the test case from that PR 
> and revert the the original hack.

The MIPS parts are OK regardless of whether the haifa-sched.c patch is.
I have to say though that the hook interface is less than ideal if we
have to introduce hacks like this.  We ought to pass can_issue_more
to sched_reorder2 if the hook needs to be aware of the current value.

The md.texi documentation implies that sched_reorder2 returns the
same thing as sched_reorder, but as you point out, it doesn't.
The sched_reorder documentation explicitly says that "issue_rate" is
usually the correct return value for that hook, but it seems that
issue_rate is never the correct return value for sched_reorder2.
Having a different prototype for sched_reorder2 (passing the extra
can_issue_more parameter) would IMO help to make this clearer.

Richard
Jie Zhang Oct. 21, 2010, 12:30 a.m. UTC | #2 
On 10/21/2010 04:44 AM, Richard Sandiford wrote:
> Jie Zhang<jie@codesourcery.com>  writes:
>> Currently TARGET_SCHED_REORDER2 returns mips_issue_rate (), which will
>> reset can_issue_more to the issue_rate after scheduling each
>> instruction. See haifa-sched.c:sched_block for the details. Because of
>> this, issue_rate is not honored when scheduling. This caused PR
>> rtl-optimization/37360 since the issue_rate is set to a value smaller
>> than the number of instructions that scheduler DFA can issue in one
>> cycle for mips sb1.
>
> The haifa-sched.c part looks good to me, but I can't approve it.
> Thanks for tracking down the underlying problem.
>
Andrey and scheduler maintainers?

>> This patch makes mips target follow rs6000 and sh. It caches
>> can_issue_more in TARGET_SCHED_VARIABLE_ISSUE and return the cached
>> value in TARGET_SCHED_REORDER2. It also addes the test case from that PR
>> and revert the the original hack.
>
> The MIPS parts are OK regardless of whether the haifa-sched.c patch is.
> I have to say though that the hook interface is less than ideal if we
> have to introduce hacks like this.  We ought to pass can_issue_more
> to sched_reorder2 if the hook needs to be aware of the current value.
>
Thanks for review!

> The md.texi documentation implies that sched_reorder2 returns the
> same thing as sched_reorder, but as you point out, it doesn't.
> The sched_reorder documentation explicitly says that "issue_rate" is
> usually the correct return value for that hook, but it seems that
> issue_rate is never the correct return value for sched_reorder2.
> Having a different prototype for sched_reorder2 (passing the extra
> can_issue_more parameter) would IMO help to make this clearer.
>
I agree. Let's just get it right for mips and then if I find some time I 
can make it clearer.


Regards,
Andrey Belevantsev Oct. 21, 2010, 8:18 a.m. UTC | #3 
On 21.10.2010 4:30, Jie Zhang wrote:
> On 10/21/2010 04:44 AM, Richard Sandiford wrote:
>> Jie Zhang<jie@codesourcery.com> writes:
>>> Currently TARGET_SCHED_REORDER2 returns mips_issue_rate (), which will
>>> reset can_issue_more to the issue_rate after scheduling each
>>> instruction. See haifa-sched.c:sched_block for the details. Because of
>>> this, issue_rate is not honored when scheduling. This caused PR
>>> rtl-optimization/37360 since the issue_rate is set to a value smaller
>>> than the number of instructions that scheduler DFA can issue in one
>>> cycle for mips sb1.
>>
>> The haifa-sched.c part looks good to me, but I can't approve it.
>> Thanks for tracking down the underlying problem.
>>
> Andrey and scheduler maintainers?
>
I'm all for the patch, life is easier for the scheduler when nobody lies. 
I cannot approve it though.  As I wrote earlier, we just need to be on the 
lookout for possible fallout on other targets which may have the same 
problem.

For an extra caution, I have tried all tests from 45352 with all flag 
combinations and -fschedule-insns[2] instead of -fselective-scheduling[2], 
and they work fine.  This is with both your other haifa-sched.c patch and 
this one applied.

Andrey
Jie Zhang Oct. 21, 2010, 8:23 a.m. UTC | #4 
On 10/21/2010 04:18 PM, Andrey Belevantsev wrote:
> On 21.10.2010 4:30, Jie Zhang wrote:
>> On 10/21/2010 04:44 AM, Richard Sandiford wrote:
>>> Jie Zhang<jie@codesourcery.com> writes:
>>>> Currently TARGET_SCHED_REORDER2 returns mips_issue_rate (), which will
>>>> reset can_issue_more to the issue_rate after scheduling each
>>>> instruction. See haifa-sched.c:sched_block for the details. Because of
>>>> this, issue_rate is not honored when scheduling. This caused PR
>>>> rtl-optimization/37360 since the issue_rate is set to a value smaller
>>>> than the number of instructions that scheduler DFA can issue in one
>>>> cycle for mips sb1.
>>>
>>> The haifa-sched.c part looks good to me, but I can't approve it.
>>> Thanks for tracking down the underlying problem.
>>>
>> Andrey and scheduler maintainers?
>>
> I'm all for the patch, life is easier for the scheduler when nobody
> lies. I cannot approve it though. As I wrote earlier, we just need to be
> on the lookout for possible fallout on other targets which may have the
> same problem.
>
Thanks. Then let's wait for review from a scheduler maintainer.

> For an extra caution, I have tried all tests from 45352 with all flag
> combinations and -fschedule-insns[2] instead of
> -fselective-scheduling[2], and they work fine. This is with both your
> other haifa-sched.c patch and this one applied.
>
Thanks for testing.


Regards,
diff mbox

Patch


	PR rtl-optimization/37360
	* config/mips/mips.c (cached_can_issue_more): New local variable.
	(mips_sched_reorder_1): New.
	(mips_sched_reorder): Use mips_sched_reorder_1.
	(mips_sched_reorder2): New.
	(mips_variable_issue): Set cached_can_issue_more.
	(TARGET_SCHED_REORDER2): Define to mips_sched_reorder2
	instead of mips_sched_reorder.

	Revert
	2008-09-09  Andrey Belevantsev  <abel@ispras.ru>
        PR rtl-optimization/37360
        * haifa-sched.c (max_issue): Do not assert that we never issue more
        insns than issue_rate.  Add comment.

	testsuite/
	* gcc.dg/pr37360.c: New test.

Index: testsuite/gcc.dg/pr37360.c
===================================================================
--- testsuite/gcc.dg/pr37360.c	(revision 0)
+++ testsuite/gcc.dg/pr37360.c	(revision 0)
@@ -0,0 +1,21 @@ 
+/* PR rtl-optimization/37360 */
+/* { dg-do compile { target fpic } } */
+/* { dg-options "-O3 -fPIC" } */
+
+typedef unsigned int UQItype __attribute__ ((mode (QI)));
+typedef unsigned int USItype __attribute__ ((mode (SI)));
+
+extern const UQItype __popcount_tab[256];
+extern int __popcountsi2 (USItype);
+
+int
+__popcountsi2 (USItype x)
+{
+  int i, ret = 0;
+
+  for (i = 0; i < (4 * 8); i += 8)
+    ret += __popcount_tab[(x >> i) & 0xff];
+
+  return ret;
+}
+
Index: haifa-sched.c
===================================================================
--- haifa-sched.c	(revision 165712)
+++ haifa-sched.c	(working copy)
@@ -2479,14 +2479,7 @@  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.
-     However, some targets (e.g. MIPS/SB1) claim lower issue rate than can be
-     achieved to get better performance.  Until these targets are fixed to use
-     scheduler hooks to manipulate insns priority instead, the assert should
-     be disabled.
-
-     gcc_assert (more_issue >= 0);  */
+  gcc_assert (more_issue >= 0);
 
   for (i = 0; i < n_ready; i++)
     if (!ready_try [i])
Index: config/mips/mips.c
===================================================================
--- config/mips/mips.c	(revision 165712)
+++ config/mips/mips.c	(working copy)
@@ -589,6 +589,10 @@  static const char *mips_hi_relocs[NUM_SY
 /* Target state for MIPS16.  */
 struct target_globals *mips16_globals;
 
+/* Cached value of can_issue_more. This is cached in mips_variable_issue hook
+   and returned from mips_sched_reorder2.  */
+static int cached_can_issue_more;
+
 /* Index R is the smallest register class that contains register R.  */
 const enum reg_class mips_regno_to_class[FIRST_PSEUDO_REGISTER] = {
   LEA_REGS,	LEA_REGS,	M16_REGS,	V1_REG,
@@ -12436,11 +12440,11 @@  mips_sched_init (FILE *file ATTRIBUTE_UN
   mips_ls2.falu1_turn_p = true;
 }
 
-/* Implement TARGET_SCHED_REORDER and TARGET_SCHED_REORDER2.  */
+/* Subroutine used by TARGET_SCHED_REORDER and TARGET_SCHED_REORDER2.  */
 
 static int
-mips_sched_reorder (FILE *file ATTRIBUTE_UNUSED, int verbose ATTRIBUTE_UNUSED,
-		    rtx *ready, int *nreadyp, int cycle ATTRIBUTE_UNUSED)
+mips_sched_reorder_1 (FILE *file ATTRIBUTE_UNUSED, int verbose ATTRIBUTE_UNUSED,
+		      rtx *ready, int *nreadyp, int cycle ATTRIBUTE_UNUSED)
 {
   if (!reload_completed
       && TUNE_MACC_CHAINS
@@ -12455,10 +12459,28 @@  mips_sched_reorder (FILE *file ATTRIBUTE
 
   if (TUNE_74K)
     mips_74k_agen_reorder (ready, *nreadyp);
+}
 
+/* Implement TARGET_SCHED_REORDER.  */
+
+static int
+mips_sched_reorder (FILE *file ATTRIBUTE_UNUSED, int verbose ATTRIBUTE_UNUSED,
+		    rtx *ready, int *nreadyp, int cycle ATTRIBUTE_UNUSED)
+{
+  mips_sched_reorder_1 (file, verbose, ready, nreadyp, cycle);
   return mips_issue_rate ();
 }
 
+/* Implement TARGET_SCHED_REORDER2.  */
+
+static int
+mips_sched_reorder2 (FILE *file ATTRIBUTE_UNUSED, int verbose ATTRIBUTE_UNUSED,
+		     rtx *ready, int *nreadyp, int cycle ATTRIBUTE_UNUSED)
+{
+  mips_sched_reorder_1 (file, verbose, ready, nreadyp, cycle);
+  return cached_can_issue_more;
+}
+
 /* Update round-robin counters for ALU1/2 and FALU1/2.  */
 
 static void
@@ -12516,6 +12538,7 @@  mips_variable_issue (FILE *file ATTRIBUT
 	      || recog_memoized (insn) < 0
 	      || get_attr_type (insn) != TYPE_MULTI);
 
+  cached_can_issue_more = more;
   return more;
 }
 
@@ -16408,7 +16431,7 @@  mips_shift_truncation_mask (enum machine
 #undef TARGET_SCHED_REORDER
 #define TARGET_SCHED_REORDER mips_sched_reorder
 #undef TARGET_SCHED_REORDER2
-#define TARGET_SCHED_REORDER2 mips_sched_reorder
+#define TARGET_SCHED_REORDER2 mips_sched_reorder2
 #undef TARGET_SCHED_VARIABLE_ISSUE
 #define TARGET_SCHED_VARIABLE_ISSUE mips_variable_issue
 #undef TARGET_SCHED_ADJUST_COST