Re: [PATCH] insn priority adjustments in scheduler and rs6000 port

[Vladimir Makarov <vmakarov at redhat dot com>]

Dorit Naishlos wrote:
> 
> Hi Vlad,
> 
> >  Another things is that the patch is about improving
> > the generated code.  Then we should have some benchmarks (the best
> > ones are SPEC) proving that the patch improves the code.
> >
> Overall we've seen 0.3%/0.2% improvement in spec fp/int (only very few
> benchmarks were degraded; maximum degradation was 1.2%, and maximum
> improvement 3.2%). Combined with the second patch (scheduling of queued
> insns) we've seen an overall 0.6% improvement in both spec int and fp
> (maximum degradation 1%, maximum imoprovement 4.1%).
> 

Great.  That is enough to approve the patch in general.

> >   But in general the patch looks ok.  So if you fix all comments and
> > give us some benchmarks, the patch could be committed in the mainline.
> >
> Attached is the revised patch, incorporating your comments.
>

Yes, that is much better.  Please go ahead and commit the new version of
the patch into the mainline.
 
> >   As for the patch about scheduling of queued insns, it is more
> > controversial.  The same comments are applicable to the patch.
> > But I need to think more about the patch idea because it violates
> > the scheduler design:
> >
> This is true, but it can be useful to allow this violation if a target
> specifically requests that. For example, if data/resource delays do not
> prevent the machine from filling up dispatch groups, the only way the
> scheduler can (try to) stay in synch with the processor behavior is to
> allow the same flexibility that the processor dispatcher has. This feature
> can be useful in a multiple-issue out-of-order machine, where (a) it's
> practically hopeless to predict the actual data/resource delays, however:
> (b) there's a better chance to predict the actual grouping that will be
> formed, and (c) correctly emulating the grouping can be very important.
> 

Yes, I understood that the patch for OOO processors where you never know
when the insn will be retired and for which issuing as much as possible
could improve the code.

> One thing we can do in order to minimize the violation of scheduler
> assumptions and maintain a more "correct" description of the delays between
> dependent insns, is to record the number of cycles by which an INSN was
> prematurely scheduled ("premature_issue"). This value will be used when
> scheduling the INSN (in schedule_insn()) to adjust the delay of each insn
> 'next' that depends upon it:
> INSN_TICK (next) =
>      MAX (INSN_TICK (next), clock + cost + premature_issue);
>                                          ^^^^^^^^^^^^^^^^^
> The value "premature_issue" models the expected delay between the issue
> cycle and the execute cycle; incorporating it this way is suitable for
> targets that have issue queues.
> 
> What do you think?

  I also thought about another solution -- removing queues and having
only ready list.  But it is even more radical than the original  patch.

  I like your new idea much better than the previous one.  You could
implement premature_issue as a hook (if it is not defined, the value
should be zero) and print its value (if it is not zero in debugging
output).  So if it is possible to get the same behaviour (and as I
understand to get the same improvement), could you rewrite the patch,
please.  I'll review it as quick as possible.  Please, don't forget
about the documentation/style/benchmarks.

Vlad