Message ID | 1423646457-9594-1-git-send-email-james.greenhalgh@arm.com |
---|---|
State | New |
Headers | show |
On 02/11/15 02:20, James Greenhalgh wrote: > > On Mon, Feb 09, 2015 at 11:16:56PM +0000, Jeff Law wrote: >> On 02/06/15 05:24, James Greenhalgh wrote: >>> >>> --- >>> 2015-02-06 James Greenhalgh <james.greenhalgh@arm.com> >>> >>> * haifa-sched.c (recompute_todo_spec): After applying a >>> replacement and cancelling a dependency, also clear the >>> SCHED_GROUP_P flag. >> My worry here would be that we might be clearing a SCHED_GROUP_P that >> had been set for some reason other than macro-fusion. > > Yeah, I also had this worry. This patch tackles the problem from the > other direction. If we see a SCHED_GROUP_P on an insn, treat it as a > hard dependency, and don't try to rewrite it. I think this will always > be "safe" but it might pessimize if the dependency breaker would have > resulted in better code generation. > > I don't think this gives you anything towards fixing your bug, but > it clears mine. Right. Mine was in the management of the ready queue. We allowed something with SCHED_GROUP_P to get deferred for several cycles. While it was deferred another insn that was previously deferred became ready and fired. That messed up the scheduling group and ultimately resulted in incorrect code. The fix was actually pretty simple, We just queue the SCHED_GROUP_P for a single cycle, then reevaluate. > > I've bootstrapped and tested on x86_64-unknown-linux-gnu with no > issues and given it a quick check on the problem code from before, > where it has the desired impact. > > Thanks, > James > > --- > 2015-02-10 James Greenhalgh <james.greenhalgh@arm.com> > > * haifa-sched.c (recompute_todo_spec): Treat SCHED_GROUP_P > as forcing a HARD_DEP between instructions, thereby > disallowing rewriting to break dependencies. OK. jeff
On Feb 17, 2015, at 9:43 AM, Jeff Law <law@redhat.com> wrote: > On 02/11/15 02:20, James Greenhalgh wrote: >> >> On Mon, Feb 09, 2015 at 11:16:56PM +0000, Jeff Law wrote: >>> On 02/06/15 05:24, James Greenhalgh wrote: >>>> >>>> --- >>>> 2015-02-06 James Greenhalgh <james.greenhalgh@arm.com> >>>> >>>> * haifa-sched.c (recompute_todo_spec): After applying a >>>> replacement and cancelling a dependency, also clear the >>>> SCHED_GROUP_P flag. >>> My worry here would be that we might be clearing a SCHED_GROUP_P that >>> had been set for some reason other than macro-fusion. >> >> Yeah, I also had this worry. This patch tackles the problem from the >> other direction. If we see a SCHED_GROUP_P on an insn, treat it as a >> hard dependency, and don't try to rewrite it. I think this will always >> be "safe" but it might pessimize if the dependency breaker would have >> resulted in better code generation. >> >> I don't think this gives you anything towards fixing your bug, but >> it clears mine. > Right. Mine was in the management of the ready queue. We allowed something with SCHED_GROUP_P to get deferred for several cycles. While it was deferred another insn that was previously deferred became ready and fired. That messed up the scheduling group and ultimately resulted in incorrect code. > > The fix was actually pretty simple, We just queue the SCHED_GROUP_P for a single cycle, then reevaluate. The way SCHED_GROUP_P instructions have been handled historically is by combination of two artifacts: (1) removing all dependencies for instructions inside SCHED_GROUP sequence but the one to next insn, and (2) maintaining a fast track for SCHED_GROUP insns that ensures that once the first SCHED_GROUP insn is issued, scheduler does nothing but issuing the single dependent insn of the current one. The side effect of (1) is that scheduling SCHED_GROUP in the normal flow will cause correctness problems (what Jeff is seeing) since some/most of the dependencies of SCHED_GROUP_P insn were removed. My educated guess is that the problem was introduced by Bernd's major reworking of the scheduler. The enforcement of (2) is now done in prune_ready_list, which doesn't seem to handle a couple of conner cases. One corner case is what happens when SCHED_GROUP insn is delayed for several cycles. The second one (that I know off) is what will happen if first instructions of two or more of separate SCHED_GROUPs become ready at the same cycle. The first corner case, I believe, used to be handled with help of last_scheduled_insn, which can't be used reliably anymore due to backtracking. The second corner case, I believe, was never handled properly. > >> >> I've bootstrapped and tested on x86_64-unknown-linux-gnu with no >> issues and given it a quick check on the problem code from before, >> where it has the desired impact. >> >> Thanks, >> James >> >> --- >> 2015-02-10 James Greenhalgh <james.greenhalgh@arm.com> >> >> * haifa-sched.c (recompute_todo_spec): Treat SCHED_GROUP_P >> as forcing a HARD_DEP between instructions, thereby >> disallowing rewriting to break dependencies. > OK. > jeff The patch looks good to me too. Once SCHED_GROUP_P is set on an insn, it becomes untouchable due to lack of complete dependency information. -- Maxim Kuvyrkov www.linaro.org
On 02/18/15 01:03, Maxim Kuvyrkov wrote: > > The way SCHED_GROUP_P instructions have been handled historically is > by combination of two artifacts: (1) removing all dependencies for > instructions inside SCHED_GROUP sequence but the one to next insn, > and (2) maintaining a fast track for SCHED_GROUP insns that ensures > that once the first SCHED_GROUP insn is issued, scheduler does > nothing but issuing the single dependent insn of the current one. The "fast track" was actually implemented by just advancing the cycle counter forward by an appropriate number of cycles of a SCHED_GROUP_P insn got queued. So the next iteration of the loop, the SCHED_GROUP_P is magically ready along with potentially other instructions that had been queued prior to the SCHED_GROUP_P insn. But that was OK (of course) because the SCHED_GROUP_P insns get priority over everything else that is ready on a particular cycle. Bernd's work broke because the SCHED_GROUP_P insn got queued, but the cycle counter only moved forward one tick. Thus previously queued insns could become ready while the SCHED_GROUP_P insn was waiting. My fix restores correctness by queuing the SCHED_GROUP_P insn for just a single cycle (it may get queued again, but that's OK as everything else that's ready in the same cycle as a queued SCHED_GROUP_P insn will get re-queued as well). It's marginally less compile-time efficient, but it was an easy, clean fix. jeff
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c index 75d2421..06444de 100644 --- a/gcc/haifa-sched.c +++ b/gcc/haifa-sched.c @@ -1233,6 +1233,11 @@ recompute_todo_spec (rtx_insn *next, bool for_backtrack) if (!sd_lists_empty_p (next, SD_LIST_HARD_BACK)) return HARD_DEP; + /* If NEXT is intended to sit adjacent to this instruction, we don't + want to try to break any dependencies. Treat it as a HARD_DEP. */ + if (SCHED_GROUP_P (next)) + return HARD_DEP; + /* Now we've got NEXT with speculative deps only. 1. Look at the deps to see what we have to do. 2. Check if we can do 'todo'. */