Reset insn priority after inc/ref replacement in haifa sched

Message ID d525a5d9-3948-563d-9afc-3a0dcbee4c25@linux.ibm.com
State New
Headers show
Series
  • Reset insn priority after inc/ref replacement in haifa sched
Related show

Commit Message

Robin Dapp Oct. 10, 2018, 4:03 p.m.
Hi,

as my last message
(https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00280.html) did not
garner much attention, I'm posting it in proper patch form this time.
The problem I'm trying to solve is that an insn's priority seems
unchanged if the priority of insns that depend on it is changed (and the
first insn's priority was computed by on of these dependent insns'
priority).  If I missed something or there is another way this should be
working, I'd like to learn about that.

A non-bootstrapped test suite run on s390 showed no regressions and an
x86 one is currently running (current HEAD didn't bootstrap for me on
x86). The actual code changes throughout SPEC2006 are minor and the
performance impact is negligible provided we do not hit a fixable bad
case as described in my last message.

Regards
 Robin

--

gcc/ChangeLog:

2018-10-10  Robin Dapp  <rdapp@linux.ibm.com>

        * haifa-sched.c (apply_replacement):
	Reset insn priority after inc/ref replacement.

Comments

Jeff Law Oct. 10, 2018, 6:57 p.m. | #1
On 10/10/18 10:03 AM, Robin Dapp wrote:
> Hi,
> 
> as my last message
> (https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00280.html) did not
> garner much attention, I'm posting it in proper patch form this time.
> The problem I'm trying to solve is that an insn's priority seems
> unchanged if the priority of insns that depend on it is changed (and the
> first insn's priority was computed by on of these dependent insns'
> priority).  If I missed something or there is another way this should be
> working, I'd like to learn about that.
> 
> A non-bootstrapped test suite run on s390 showed no regressions and an
> x86 one is currently running (current HEAD didn't bootstrap for me on
> x86). The actual code changes throughout SPEC2006 are minor and the
> performance impact is negligible provided we do not hit a fixable bad
> case as described in my last message.
> 
> Regards
>  Robin
> 
> --
> 
> gcc/ChangeLog:
> 
> 2018-10-10  Robin Dapp  <rdapp@linux.ibm.com>
> 
>         * haifa-sched.c (apply_replacement):
> 	Reset insn priority after inc/ref replacement.
> 
See my last message.  I find myself wondering if we need to reset
INSN_PRIORITY_STATUS in update_insn_after_change and/or calling
update_insn_after_change on INSN in additional to calling it on DESC->insn.

jeff
Robin Dapp Oct. 15, 2018, noon | #2
Hi,

> See my last message.  I find myself wondering if we need to reset
> INSN_PRIORITY_STATUS in update_insn_after_change and/or calling
> update_insn_after_change on INSN in additional to calling it on DESC->insn.

I tried calling update_insn_after_change even before sending my message
but it seems to modify variables that are assumed to be set at this
stage of the algorithm (resetting INSN_TICK (insn) and INSN_COST (insn)
causes ICEs in other places).  It suffices, however, to call priority
like in the attached patch to achieve the same result.

Test suite is still running.

Regards
 Robin

gcc/ChangeLog:

2018-10-15  Robin Dapp  <rdapp@linux.ibm.com>

	* haifa-sched.c (priority): Add force_recompute parameter.
	(apply_replacement):
	Call priority () with force_recompute = true.
	(restore_pattern): Likewise.
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 1fdc9df9fb2..8aab37a2ba8 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -1590,7 +1590,7 @@ bool sched_fusion;
 
 /* Compute the priority number for INSN.  */
 static int
-priority (rtx_insn *insn)
+priority (rtx_insn *insn, bool force_recompute)
 {
   if (! INSN_P (insn))
     return 0;
@@ -1598,7 +1598,7 @@ priority (rtx_insn *insn)
   /* We should not be interested in priority of an already scheduled insn.  */
   gcc_assert (QUEUE_INDEX (insn) != QUEUE_SCHEDULED);
 
-  if (!INSN_PRIORITY_KNOWN (insn))
+  if (force_recompute || !INSN_PRIORITY_KNOWN (insn))
     {
       int this_priority = -1;
 
@@ -1695,6 +1695,12 @@ priority (rtx_insn *insn)
 
   return INSN_PRIORITY (insn);
 }
+
+static int
+priority (rtx_insn *insn)
+{
+  return priority (insn, false);
+}
 
 /* Macros and functions for keeping the priority queue sorted, and
    dealing with queuing and dequeuing of instructions.  */
@@ -4713,7 +4719,12 @@ apply_replacement (dep_t dep, bool immediately)
       success = validate_change (desc->insn, desc->loc, desc->newval, 0);
       gcc_assert (success);
 
+      rtx_insn *insn = DEP_PRO (dep);
+
+      /* Recompute priority since dependent priorities have changed.  */
+      priority (insn, true);
       update_insn_after_change (desc->insn);
+
       if ((TODO_SPEC (desc->insn) & (HARD_DEP | DEP_POSTPONED)) == 0)
 	fix_tick_ready (desc->insn);
 
@@ -4767,7 +4778,13 @@ restore_pattern (dep_t dep, bool immediately)
 
       success = validate_change (desc->insn, desc->loc, desc->orig, 0);
       gcc_assert (success);
+
+      rtx_insn *insn = DEP_PRO (dep);
+      /* Recompute priority since dependent priorities have changed.  */
+      priority (insn, true);
+
       update_insn_after_change (desc->insn);
+
       if (backtrack_queue != NULL)
 	{
 	  backtrack_queue->replacement_deps.safe_push (dep);
Alexander Monakov Oct. 15, 2018, 2:07 p.m. | #3
On Mon, 15 Oct 2018, Robin Dapp wrote:
> 	* haifa-sched.c (priority): Add force_recompute parameter.
> 	(apply_replacement):
> 	Call priority () with force_recompute = true.
> 	(restore_pattern): Likewise.

A C++ style nit/question: instead of adding a new overload 

  priority (rtx_insn *, bool)

you can add a parameter with a default value in the existing
static function

  priority (rtx_insn *insn, bool force_recompute = false)

unless I'm missing something and the new overload is on purpose?

Alexander
Robin Dapp Oct. 16, 2018, 6:48 a.m. | #4
> A C++ style nit/question: instead of adding a new overload 
> 
>   priority (rtx_insn *, bool)
> 
> you can add a parameter with a default value in the existing
> static function
> 
>   priority (rtx_insn *insn, bool force_recompute = false)

Sometimes I'm still stuck in C land with GCC :), thanks will change this
if the rest of the patch is ok.

Regards
 Robin
Jeff Law Oct. 16, 2018, 2:11 p.m. | #5
On 10/16/18 12:48 AM, Robin Dapp wrote:
>> A C++ style nit/question: instead of adding a new overload 
>>
>>   priority (rtx_insn *, bool)
>>
>> you can add a parameter with a default value in the existing
>> static function
>>
>>   priority (rtx_insn *insn, bool force_recompute = false)
> 
> Sometimes I'm still stuck in C land with GCC :), thanks will change this
> if the rest of the patch is ok.
OK.
jeff
Robin Dapp Oct. 18, 2018, 8:10 a.m. | #6
Hi,

I added a check before calling priority in restore_pattern.  In the last
version, not checking that would lead to assertion failure in priority
since the insn might already have been scheduled.

Bootstrapped and regtested on x86_64 and ppc8, regtested on s390x.

Regards
 Robin

--

gcc/ChangeLog:

2018-10-16  Robin Dapp  <rdapp@linux.ibm.com>

	* haifa-sched.c (priority): Add force_recompute parameter.
	(apply_replacement): Call priority () with force_recompute = true.
	(restore_pattern): Likewise.
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 1fdc9df9fb2..2c84ce38143 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -830,7 +830,7 @@ add_delay_dependencies (rtx_insn *insn)
 
 /* Forward declarations.  */
 
-static int priority (rtx_insn *);
+static int priority (rtx_insn *, bool force_recompute = false);
 static int autopref_rank_for_schedule (const rtx_insn *, const rtx_insn *);
 static int rank_for_schedule (const void *, const void *);
 static void swap_sort (rtx_insn **, int);
@@ -1590,7 +1590,7 @@ bool sched_fusion;
 
 /* Compute the priority number for INSN.  */
 static int
-priority (rtx_insn *insn)
+priority (rtx_insn *insn, bool force_recompute)
 {
   if (! INSN_P (insn))
     return 0;
@@ -1598,7 +1598,7 @@ priority (rtx_insn *insn)
   /* We should not be interested in priority of an already scheduled insn.  */
   gcc_assert (QUEUE_INDEX (insn) != QUEUE_SCHEDULED);
 
-  if (!INSN_PRIORITY_KNOWN (insn))
+  if (force_recompute || !INSN_PRIORITY_KNOWN (insn))
     {
       int this_priority = -1;
 
@@ -4713,7 +4713,12 @@ apply_replacement (dep_t dep, bool immediately)
       success = validate_change (desc->insn, desc->loc, desc->newval, 0);
       gcc_assert (success);
 
+      rtx_insn *insn = DEP_PRO (dep);
+
+      /* Recompute priority since dependent priorities may have changed.  */
+      priority (insn, true);
       update_insn_after_change (desc->insn);
+
       if ((TODO_SPEC (desc->insn) & (HARD_DEP | DEP_POSTPONED)) == 0)
 	fix_tick_ready (desc->insn);
 
@@ -4767,7 +4772,17 @@ restore_pattern (dep_t dep, bool immediately)
 
       success = validate_change (desc->insn, desc->loc, desc->orig, 0);
       gcc_assert (success);
+
+      rtx_insn *insn = DEP_PRO (dep);
+
+      if (QUEUE_INDEX (insn) != QUEUE_SCHEDULED)
+	{
+	  /* Recompute priority since dependent priorities may have changed.  */
+	  priority (insn, true);
+	}
+
       update_insn_after_change (desc->insn);
+
       if (backtrack_queue != NULL)
 	{
 	  backtrack_queue->replacement_deps.safe_push (dep);
Jeff Law Oct. 18, 2018, 5 p.m. | #7
On 10/18/18 2:10 AM, Robin Dapp wrote:
> Hi,
> 
> I added a check before calling priority in restore_pattern.  In the last
> version, not checking that would lead to assertion failure in priority
> since the insn might already have been scheduled.
> 
> Bootstrapped and regtested on x86_64 and ppc8, regtested on s390x.
> 
> Regards
>  Robin
> 
> --
> 
> gcc/ChangeLog:
> 
> 2018-10-16  Robin Dapp  <rdapp@linux.ibm.com>
> 
> 	* haifa-sched.c (priority): Add force_recompute parameter.
> 	(apply_replacement): Call priority () with force_recompute = true.
> 	(restore_pattern): Likewise.
> 
Still OK :-)
jeff
Robin Dapp Oct. 19, 2018, 7:14 a.m. | #8
> Still OK :-)

Committed as r265304.

Regards
 Robin

Patch

commit 69ade62116eecf0a2bdf1cb099158e6d1dca1f3f
Author: Robin Dapp <rdapp@linux.ibm.com>
Date:   Mon Oct 8 15:26:58 2018 +0200

    recompute priority of insn whose dependencies have changed after applying an inc/ref replacement.

diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 1fdc9df9fb2..1a7fe7b78e2 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -4713,7 +4713,21 @@  apply_replacement (dep_t dep, bool immediately)
       success = validate_change (desc->insn, desc->loc, desc->newval, 0);
       gcc_assert (success);
 
+      rtx_insn *insn = DEP_PRO (dep);
+
+      INSN_PRIORITY_STATUS (insn) = -1;
+
+      sd_iterator_def sd_it;
+      sd_it = sd_iterator_start (insn, SD_LIST_FORW);
+      while (sd_iterator_cond (&sd_it, &dep))
+	{
+	  DEP_COST (dep) = UNKNOWN_DEP_COST;
+	  sd_iterator_next (&sd_it);
+	}
+
+      priority (insn);
       update_insn_after_change (desc->insn);
+
       if ((TODO_SPEC (desc->insn) & (HARD_DEP | DEP_POSTPONED)) == 0)
 	fix_tick_ready (desc->insn);