Re: [PATCH] Fix instability of -fschedule-insn for x86

[Igor Zamyatin <izamyatin at gmail dot com>]

We also plan to test these changes along with LRA

On Sun, Sep 30, 2012 at 4:33 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Sep 18, 2012 at 1:31 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>
>>> This patch aims to fix all stability issues related to using the first
>>> scheduler in gcc
>>> for x86 target (there several reported issues related to this problem).
>>>
>>> Main idea of this activity is mostly to provide user a possibility to
>>> safely turn on first scheduler for his codes. In some cases this could
>>> positively affect performance, especially for in-order Atom.
>>>
>>> Below is short description of proposed changes.
>>
>>> 2012-09-18  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>
>>>         * config/i386/i386.c (ix86_dep_by_shift_count_body) : Add
>>>         check on reload_completed since it can be invoked before
>>>         register allocation phase in 1st scheduler.
>>>         (ia32_multipass_dfa_lookahead) : Do not use dfa_lookahead for 1st
>>>         Scheduler to save compile time.
>>>         (ix86_sched_reorder) : Do not perform ready list reordering for 1st
>>>         Scheduler to save compile time.
>>>         (insn_is_function_arg) : New function. Returns true if lhs of insn is
>>>         HW function argument register.
>>>         (add_parameter_dependencies) : New function. Add output dependencies
>>>         for chain of function adjacent arguments if only there is a move to
>>>         likely spilled HW registers. Return first argument if at least one
>>>         dependence was added or NULL otherwise.
>>>         (avoid_func_arg_motion) : New function. Add output or anti dependency
>>>         from insn to first_arg to restrict code motion.
>>>         (add_dependee_for_func_arg) : New function. Avoid cross block motion of
>>>         function argument through adding dependency from the first non-jump
>>>         insn in bb.
>>>         (ix86_dependencies_evaluation_hook) : New function. Hook for schedule1:
>>>         avoid motion of function arguments passed in passed in likely spilled
>>>         HW registers.
>>>         (ix86_adjust_priority) : New function. Hook for schedule1: set priority
>>>         of moves from likely spilled HW registers to maximum to schedule them
>>>         as soon as possible.
>>>         (ix86_sched_init_global): Do not perform multipass scheduling for 1st
>>>         Scheduler to save compile time.
>>
>> I would kindly ask scheduler expert to review the patch from the
>> scheduler functionality POV.
>
> I have received opinion from Vladimir from off-line discussion, quoted below:
>
> --quote--
> I think, it is ok.
>
>   Switching off first cycle multipass scheduling is ok.  It is mostly
> useful when the order of insns issued on the same cycle is important
> (mostly VLIW or quasy-VLIW processors).
>
>   Other solutions are necessary to decrease spills and avoid reload
> crash (can not find a register in a class) when the 1st insn
> scheduling is on.  I don't think it fully avoids possibility of the
> reload crashes but it takes into account most of cases resulting in
> the crashes and makes the crash possibility really negligible.
> Register pressure sensitive insn scheduling decreased the possibility.
>  This patch will make it negligible.  And LRA will solve all the rest
> cases of the crashes.
>
>   I don't like a bit absence in freedom of moving argument insns with
> likely spilled hard-regs between each other as they are chained in the
> original order but it is debatable because it still decreases the
> possibility of spills.
>
>   In overall, the patch is ok for me.
> --/quote--
>
> Based on this opinion, the patch is OK for mainline, if there are no
> objections from other x86 maintainers in the next couple of days
> (48h). However, please watch for possible fallout from the patch,
> compile-time ICEs and performance problems. x86 and scheduler didn't
> play well together in the past, but your patch and (in the near
> future) LRA seems to fix all these problems.
>
> Thanks,
> Uros.