Re: Questions about selective scheduler and PowerPC

[Jie Zhang <jie at codesourcery dot com>]

On 10/19/2010 10:16 PM, Andrey Belevantsev wrote:
> On 19.10.2010 17:57, Jie Zhang wrote:
> > Removing the failing assert fixes the test case. But I wonder why not
> > just
> > get max_issue correct. I'm testing the attached patch. IMHO, max_issue
> > looks confusing.
> >
> > * The concept of ISSUE POINT has never been used since the code landed in
> > repository.
> >
> > * In the comment just before the function, it's mentioned that MAX_POINTS
> > is the sum of points of all instructions in READY. But it does not match
> > the code. The code only summarizes the points of the first MORE_ISSUE
> > instructions. If later ISSUE_POINTS become not uniform, that piece of
> > code
> > should be redesigned.
> >
> > So I think it's good to remove it now. And "top - choice_stack" is a good
> > replacement for top->n. So we can remove field n from struct
> > choice_entry,
> > too.
> >
> > Now I'm looking at MIPS target to find out why this change in the would
> > cause PR37360.
> I agree that ISSUE_POINTS can be removed, as it was not used (maybe
> Maxim can comment more on this). However, the assert is not about the
> points but exactly about the situation when a target is lying to the
> compiler about its issue rate.
>
> The ideal situation is that we agree on that this should never happen,
> but then you need to fix all targets that use this trick, and it seems
> that there is at least mips, ppc, and x86-64 (which is why I pointed you
> to 45352). The fix would be to find out why claiming the true issue rate
> degrades performance and to implement the proper scheduling hooks for
> changing priority of some insns, or to enable -fsched-pressure for the
> offending targets.
I agree. But I still have a question about TARGET_SCHED_ISSUE_RATE. 
According to my understanding of gccint:

[quote]
Target Hook: int TARGET_SCHED_ISSUE_RATE (void)
[snip]
Although the insn scheduler can define itself the possibility of issue 
an insn on the same cycle, the value can serve as an additional 
constraint to issue insns on the same simulated processor cycle
[snip]
[/quote]

it should be allowed to be defined smaller than the issue rate defined 
by the scheduler DFA. So even if the backend defines a DFA which is 
capable to issue 4 instructions in one cycle but it also defines 
TARGET_SCHED_ISSUE_RATE to 3, the scheduler should restrict the number 
of instructions issued in one cycle to 3 instead of 4.

So I think this assert should hold even the backend lies to scheduler 
about the issue rate. Fixing the lies is another problem.

With the attached draft patch, we can enable the assert in max_issue 
without regression on PR37360.

> This is a lot of work, which is why this assert was installed in
> max_issue for relatively short amount of time. Maybe it's time to try
> again, but let's have a consensus first that this assert should never
> trigger by design and we have enough flexibility in the scheduler to
> provide legal means to achieve the same performance effect.
Agree.


Regards,
--
Jie Zhang
CodeSourcery

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index b13d648..7653941 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -589,6 +589,10 @@ static const char *mips_hi_relocs[NUM_SYMBOL_TYPES];
 /* 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,
@@ -12439,8 +12443,8 @@ mips_sched_init (FILE *file ATTRIBUTE_UNUSED, int verbose ATTRIBUTE_UNUSED,
 /* Implement 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,25 @@ mips_sched_reorder (FILE *file ATTRIBUTE_UNUSED, int verbose ATTRIBUTE_UNUSED,
 
   if (TUNE_74K)
     mips_74k_agen_reorder (ready, *nreadyp);
+}
 
+
+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 ();
 }
 
+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 +12535,7 @@ mips_variable_issue (FILE *file ATTRIBUTE_UNUSED, int verbose ATTRIBUTE_UNUSED,
 	      || recog_memoized (insn) < 0
 	      || get_attr_type (insn) != TYPE_MULTI);
 
+  cached_can_issue_more = more;
   return more;
 }
 
@@ -16408,7 +16428,7 @@ mips_shift_truncation_mask (enum machine_mode mode)
 #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