On Feb 17, 2015, at 9:43 AM, Jeff Law <l...@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.greenha...@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.greenha...@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
