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

[Uros Bizjak <ubizjak at gmail dot com>]

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.