scheduler bug fix for AArch64 insn fusing SCHED_GROUP usage

Message ID CABXYE2Xqc4imhDUmM_3ap-Jf-xuY5ZTTPB1jCYGigbWB9iozRA@mail.gmail.com
State New
Headers show

Commit Message

Jim Wilson July 13, 2017, 10 p.m.
The AArch64 port uses SCHED_GROUP to mark instructions that get fused
at issue time, to ensure that they will be issued together.  However,
in the scheduler, use of a SCHED_GROUP forces all other instructions
to issue in the next cycle.  This is wrong for AArch64 ports using
insn fusing which can issue multiple insns per cycle, as aarch64
SCHED_GROUP insns can all issue in the same cycle, and other insns can
issue in the same cycle also.

I put a testcase and some info in bug 81434.
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81434

The attached patch fixes the problem.  The behavior in pass == 0 is
same as now.  All non sched group insns are ignored, and all sched
group insns are checked to see if they need to be queued for a latter
cycle.  The difference is in the second pass where non sched group
insns are queued for a latter cycle only if there is a sched group
insn that got queued.  Since sched group insns always sort to the top
of the list of insns to schedule, all sched group insns still get
scheduled together as before.

This has been tested with an Aarch64 bootstrap and make check.

OK?

Jim

Comments

Jim Wilson July 21, 2017, 10:09 p.m. | #1
Ping.

https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00779.html

On Thu, Jul 13, 2017 at 3:00 PM, Jim Wilson <jim.wilson@linaro.org> wrote:
> The AArch64 port uses SCHED_GROUP to mark instructions that get fused

> at issue time, to ensure that they will be issued together.  However,

> in the scheduler, use of a SCHED_GROUP forces all other instructions

> to issue in the next cycle.  This is wrong for AArch64 ports using

> insn fusing which can issue multiple insns per cycle, as aarch64

> SCHED_GROUP insns can all issue in the same cycle, and other insns can

> issue in the same cycle also.

>

> I put a testcase and some info in bug 81434.

>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81434

>

> The attached patch fixes the problem.  The behavior in pass == 0 is

> same as now.  All non sched group insns are ignored, and all sched

> group insns are checked to see if they need to be queued for a latter

> cycle.  The difference is in the second pass where non sched group

> insns are queued for a latter cycle only if there is a sched group

> insn that got queued.  Since sched group insns always sort to the top

> of the list of insns to schedule, all sched group insns still get

> scheduled together as before.

>

> This has been tested with an Aarch64 bootstrap and make check.

>

> OK?

>

> Jim
Jim Wilson Sept. 5, 2017, 6:04 p.m. | #2
ping^2

On Fri, Jul 21, 2017 at 3:09 PM, Jim Wilson <jim.wilson@linaro.org> wrote:
> Ping.

>

> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00779.html

>

> On Thu, Jul 13, 2017 at 3:00 PM, Jim Wilson <jim.wilson@linaro.org> wrote:

>> The AArch64 port uses SCHED_GROUP to mark instructions that get fused

>> at issue time, to ensure that they will be issued together.  However,

>> in the scheduler, use of a SCHED_GROUP forces all other instructions

>> to issue in the next cycle.  This is wrong for AArch64 ports using

>> insn fusing which can issue multiple insns per cycle, as aarch64

>> SCHED_GROUP insns can all issue in the same cycle, and other insns can

>> issue in the same cycle also.

>>

>> I put a testcase and some info in bug 81434.

>>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81434

>>

>> The attached patch fixes the problem.  The behavior in pass == 0 is

>> same as now.  All non sched group insns are ignored, and all sched

>> group insns are checked to see if they need to be queued for a latter

>> cycle.  The difference is in the second pass where non sched group

>> insns are queued for a latter cycle only if there is a sched group

>> insn that got queued.  Since sched group insns always sort to the top

>> of the list of insns to schedule, all sched group insns still get

>> scheduled together as before.

>>

>> This has been tested with an Aarch64 bootstrap and make check.

>>

>> OK?

>>

>> Jim
Jim Wilson Oct. 11, 2017, 3:52 a.m. | #3
On Sun, 2017-09-10 at 19:45 +0200, Jim Wilson wrote:
> ---------- Forwarded message ----------

> From: Jim Wilson <jim.wilson@linaro.org>

> Date: Tue, Sep 5, 2017 at 8:04 PM

> Subject: Re: [PATCH] scheduler bug fix for AArch64 insn fusing

> SCHED_GROUP usage

> To: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>

> Cc: Jim Wilson <jim.wilson@linaro.org>

> 

> 

> ping^2

> 

> On Fri, Jul 21, 2017 at 3:09 PM, Jim Wilson <jim.wilson@linaro.org>

> wrote:

> > 

> > Ping.

> > 

> > https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00779.html

> > 


Silly me.  I noticed that I am still listed as a sched maintainer,
which means I can self approve this.

I reverified it with x86_64 and aarch64 bootstraps and make checks.  I
also did a C only testsuite run for avr, which appears to be the best
maintained cc0 target.  There were no regressions for any of the 3
targets.

The patch was applied.

Jim
2017-10-10  Jim Wilson  <wilson@tuliptree.org>

	PR rtl-optimization/81434
	* haifa-sched.c (prune_ready_list): Init min_cost_group to 0.  Update
	comment for main loop.  In sched_group_found if, also add checks for
	pass and min_cost_group.

Index: haifa-sched.c
===================================================================
--- haifa-sched.c	(revision 253627)
+++ haifa-sched.c	(revision 253628)
@@ -6303,7 +6303,7 @@ prune_ready_list (state_t temp_state, bool first_c
 {
   int i, pass;
   bool sched_group_found = false;
-  int min_cost_group = 1;
+  int min_cost_group = 0;
 
   if (sched_fusion)
     return;
@@ -6319,8 +6319,8 @@ prune_ready_list (state_t temp_state, bool first_c
     }
 
   /* Make two passes if there's a SCHED_GROUP_P insn; make sure to handle
-     such an insn first and note its cost, then schedule all other insns
-     for one cycle later.  */
+     such an insn first and note its cost.  If at least one SCHED_GROUP_P insn
+     gets queued, then all other insns get queued for one cycle later.  */
   for (pass = sched_group_found ? 0 : 1; pass < 2; )
     {
       int n = ready.n_ready;
@@ -6333,7 +6333,8 @@ prune_ready_list (state_t temp_state, bool first_c
 	  if (DEBUG_INSN_P (insn))
 	    continue;
 
-	  if (sched_group_found && !SCHED_GROUP_P (insn))
+	  if (sched_group_found && !SCHED_GROUP_P (insn)
+	      && ((pass == 0) || (min_cost_group >= 1)))
 	    {
 	      if (pass == 0)
 		continue;
Maxim Kuvyrkov Oct. 16, 2017, 12:14 p.m. | #4
> On Oct 11, 2017, at 6:52 AM, Jim Wilson <wilson@tuliptree.org> wrote:

> 

> On Sun, 2017-09-10 at 19:45 +0200, Jim Wilson wrote:

>> ---------- Forwarded message ----------

>> From: Jim Wilson <jim.wilson@linaro.org>

>> Date: Tue, Sep 5, 2017 at 8:04 PM

>> Subject: Re: [PATCH] scheduler bug fix for AArch64 insn fusing

>> SCHED_GROUP usage

>> To: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>

>> Cc: Jim Wilson <jim.wilson@linaro.org>

>> 

>> 

>> ping^2

>> 

>> On Fri, Jul 21, 2017 at 3:09 PM, Jim Wilson <jim.wilson@linaro.org>

>> wrote:

>>> 

>>> Ping.

>>> 

>>> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00779.html

>>> 

> 

> Silly me.  I noticed that I am still listed as a sched maintainer,

> which means I can self approve this.

> 

> I reverified it with x86_64 and aarch64 bootstraps and make checks.  I

> also did a C only testsuite run for avr, which appears to be the best

> maintained cc0 target.  There were no regressions for any of the 3

> targets.

> 

> The patch was applied.


FWIW, the patch looks good.

--
Maxim Kuvyrkov
www.linaro.org

Patch

2017-07-13  Jim Wilson  <jim.wilson@linaro.org>

	PR rtl-optimization/81434
	* haifa-sched.c (prune_ready_list): Init min_cost_group to 0.  Update
	comment for main loop.  In sched_group_found if, also add checks for
	pass and min_cost_group.

diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 1b13e32..f6369d9 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -6300,7 +6300,7 @@  prune_ready_list (state_t temp_state, bool first_cycle_insn_p,
 {
   int i, pass;
   bool sched_group_found = false;
-  int min_cost_group = 1;
+  int min_cost_group = 0;
 
   if (sched_fusion)
     return;
@@ -6316,8 +6316,8 @@  prune_ready_list (state_t temp_state, bool first_cycle_insn_p,
     }
 
   /* Make two passes if there's a SCHED_GROUP_P insn; make sure to handle
-     such an insn first and note its cost, then schedule all other insns
-     for one cycle later.  */
+     such an insn first and note its cost.  If at least one SCHED_GROUP_P insn
+     gets queued, then all other insns get queued for one cycle later.  */
   for (pass = sched_group_found ? 0 : 1; pass < 2; )
     {
       int n = ready.n_ready;
@@ -6330,7 +6330,8 @@  prune_ready_list (state_t temp_state, bool first_cycle_insn_p,
 	  if (DEBUG_INSN_P (insn))
 	    continue;
 
-	  if (sched_group_found && !SCHED_GROUP_P (insn))
+	  if (sched_group_found && !SCHED_GROUP_P (insn)
+	      && ((pass == 0) || (min_cost_group >= 1)))
 	    {
 	      if (pass == 0)
 		continue;