Re: [Haifa Scheduler] Fix latent bug in macro-fusion/instruction grouping

[Maxim Kuvyrkov <maxim dot kuvyrkov at linaro dot org>]

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