Message ID | 4CBF02B4.4010207@codesourcery.com |
---|---|
State | New |
Headers | show |
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
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,
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
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,
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