| 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 | expand |
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
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);
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
> 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
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
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);
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
> Still OK :-)
Committed as r265304.
Regards
Robin
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);
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.