This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Patch for AMD Dispatch Scheduler


On 07/08/2010 03:34 PM, Yazdani, Reza wrote:

> This patch is an implementation of the Dispatch Scheduler in upcoming
> AMD Bulldozer processor. It is implemented as an extension to Haifa
> scheduler.
>
>
>
> Dispatch scheduling is a new BD feature. It is composed of two parts:
> the scheduling part and the alignment part.
>
>
>
> The scheduling part (this patch) arranges instructions to maximize the
> throughput of the hardware dispatcher. It makes sure dispatch widow
> boundaries are roughly observed. It is roughly, because the lengths of
> instructions, in number of bytes, are not known at the scheduling
> time. In x86 some instruction lengths may not be known until assembly
> time where information such as branch offsets are computed. Scheduling
> part is called once before register allocation and once after register
> allocation.
>
>
>
> The alignment part (not in this patch) makes sure dispatch widows
> align at the correct boundaries.
>
>
>
> A new command flag –fdispatch-sched is defined. This flag sets a new
> flag_dispatch_sched which is a default if “-march=bdver1” is selected
> on the command line.
>
>
>

Reza, sorry for the delay with the answer. As I understand, this is
your 1st non-trivial patch for GCC. The patch itself is not bad for
the 1st try. But there are a lot things to fix up.

First of all, you are trying to implement machine-dependent
optimizations in machine-independent part of compiler. File is
dispatch-sched.c is purely machine-dependent. It can be used only
for the Bulldozer processor. So it should be moved to config/i386/
directory as a part of i386.c or as a separate file. Respectively,
you should define not machine-independent flag -fdispatch-sched but
machine-dependent -mdispatch-sched. The dispatch-sched.c should
interact with haifa scheduler through machine dependent hooks (e.g. as
adjust_cost hooks in haifa-sched.c). If we have a lot of processors of
different targets with analogous problems and dispatch-sched.c is well
parameterized, we could move it to the machine-independent part (but honestly
I don't see it is coming in near future).


After the patch rework, you can resubmit the patch and I'll review parts
concerning haifa-sched.c. The machine-dependent parts (in
config/i386) should review a x86/x86_64 target maintainer (e.g. Uros
Bizjak or Jan Hubicka).

In general, the approach is ok although I think it could be solved by
DFA (as analogous problems were solved for ia64) but I am not insisting
to use DFA for this.

As for the patch itself, you should read GNU coding standards and
follow them more accurately. For example, { should be always on a
separate line including for structures and enums. All functions should
have comments and the comments should describe meaning of the function
parameters.

ChangeLog should be complete: all changed files should be mentioned
(e.g. missed common.opt or doc/imvoke.texi) and all changes in the file
should be mentioned (e.g. haifa-sched.c has new function
print_ready_dispatch and debug_ready_dispatch which are not
mentioned). You should write the change itself but not what it does
(e.g instead of "removes from ready list first candidate satisfying
dispatch condition" for ready_remove_first_dispatch you should just
write "New function"). You should use a capital letter after : in the
changelog file.



> Testing
>
> -------
>
>
>
> Self compile ran with “–fdispatch-sched –O3”. No new test added for
> this implementation. Make check of i386 tests passes. No difference in
> the number of failures with and without the dispatch flag.
>
>
>
> ChangeLog
>
> ---------
>
>
>
> 2010-06-29 Reza Yazdani <reza.yazdani@amd.com>
>
> * dispatch-sched.c: New file.
>
> * haifa-sched.c: Add support for dispatch scheduling for bdver1.
>
> (ready_remove_first_dispatch): removes from ready list first
>
> candidate satisfying dispatch condition.
>
> (choose_ready): choose the best candidate suitable for dispatch
>
> window and the scheduler.
>
> (sched_init): initialize dispatch scheduling data structures.
>
> * common.h: fdispatch-sched command line flag added.
>
> * sched-init.h: external function declarations added.
>
> * Makefile.in: dispatch-sched.c added.
>
> * config/i386/i386.c: flag_dispatch_sched set for bdver1.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]