Re: [PATCH] Improve scheduling at the end of basic blocks

[Vladimir Makarov <vmakarov at redhat dot com>]

Maxim Kuvyrkov wrote:
>  
>
> The first attached patch improves scheduling at the end of basic 
> blocks by making scheduler take into account the fact that some 
> instructions force end of the cycle (the last instruction of a basic 
> blocks being the most common case).
>
> OK for trunk?

 Maxim, sorry for the delay with the review.  The changes were so not
obvious to me that it took some time to investigate them.

 As for the first patch.  I added abort before 'return true' in
sched-rgn::rgn_insn_finishes_block_p and I've never get abort on
compilation of big set of programs (about 1M lines of C) on ppc64.
The reason is in that schedule_more_p already checks this before
max_issue.  So the patch is useless.

 As for the second patch, you incorrectly understand df_look_ahead.
It is just a constraint to the local search being done.  Comments to
max_lookahead_tries say about this constraint by defining worst
algorithm complexity.  Probably because you did not correctly
understand the parameter meaning, you enormously increase the local
search which results in up 6 times bigger compiler time on one small
test (shuffle) on Itanium or in 2 times on more widely known linpack.  The
problem is in that you don't decrease `rest' for current stack entry
(only on the new stack entry) as it was in the original code.  It
results in trying all ready elements on each recursion level.

 So I can not approve the patches.  One patch is really useless and
makes the complicated code even more complicated.  Another patch
brings more harm than benefits.  I think you should pay more attention
to benchmarking of performance patches.


> The second patch fixes handling of internal to max_issue() data 
> structure choice_stack.  Specifically, the 'rest' field of this 
> structure is handled funny.
>
>   /* The number of the rest insns whose issues we should try.  */
>   int rest;
>
> As far as I understand this field is to track how many instruction the 
> procedure has left to try to issue this cycle.  If dfa_lookahead is 2, 
> max_issue() should happily exit after 2 instructions have been 
> accepted by DFA and not try to stuff more instructions into it.
>
>   top->rest = dfa_lookahead;
>
> ...
>
>           if (state_dead_lock_p (state))
>         top->rest = 0;
>           else
>         top->rest--;
>
> ...
>
>           /* Advance to the next choice_entry.  */
>           top++;
>           /* Initialize it.  */
>           top->rest = dfa_lookahead;
>
> Note, when state_dead_lock_p() returns 'true', max_issue tries to 
> issue instructions from the ready_list anyway.
>
> The second patch makes max_issue() handle top->rest with respect to my 
> understanding outlined above; maybe there's something I've missed and 
> it is intended to handle top->rest this way.