Re: [PATCH] Implementing Swing Modulo Scheduling in GCC

[Mostafa Hagog <MUSTAFA at il dot ibm dot com>]

Vlad, Sebastian,
Thanks for your comments. We addressed all of them and updated the patch
along
with other minor corrections, including changing passes.c so that
sms_schedule
doesn't require "-fschedule-insns". Bootstrap and regression tests passed
on powepc-apple-darwin7.2.0 and i686-pc-linux-gnu.

2004-05-03  Ayal Zaks  <zaks@il.ibm.com>
      Mostafa Hagog  <mustafa@il.ibm.com>

      * Makefile.in (modulo-sched.o, ddg.o): New.
      * ddg.h, ddg.c, modulo-sched.c: New files.
      * cfglayout.c (duplicate_insn_chain): Remove "static" and push
      internals to "dupicate_insn".
      (duplicate_insn): New function.
      * cfglayout.h (duplicate_insn_chain, duplicate_insn): New
      declarations.
      * common.opt (fmodulo-sched): New flag.
      * df.c (df_bb_regno_last_use_find, df_bb_regno_first_def_find):
      Remove static and forward declaration.
      (df_find_def, df_reg_used, df_bb_regno_last_def_find): New
      functions.
      * df.h (df_bb_regno_last_use_find, df_bb_regno_first_def_find,
      df_bb_regno_last_def_find, df_find_def, df_reg_used): New
      declarations.
      * flags.h (flag_modulo_sched): New flag.
      * opts.c (common_handle_option): Handle modulo-sched flag.
      * params.def (max-sms-loop-number, sms-max-ii-factor,
      sms-dfa-history, sms-loop-average-count-threshold): New
      parameters.
      * params.h (MAX_SMS_LOOP_NUMBER, SMS_MAX_II_FACTOR,
      SMS_DFA_HISTORY, SMS_LOOP_AVERAGE_COUNT_THRESHOLD): New
      parameters.
      * passes.c ("sms", "sms-vcg"): New dumps.
      (rest_of_handle_sched): Call sms_schedule.
      * rtl.h (sms_schedule): New declaration.
      * timevar.def (TV_SMS): New.
      * toplev.c (flag_modulo_sched): Initialize.
      (f_options): Handle -fmodulo-sched option.
      * docs/invoke.texi: Document -fmodulo-sched option.
      * docs/passes.texi: Document new SMS pass.

(See attached file: gcc3.5.patch)(See attached file: modulo-sched.c)(See
attached file: ddg.c)(See attached file: ddg.h)

Vladimir Makarov <vmakarov@redhat.com> wrote on 22/04/2004 21:28:18:

>
>
> > We addressed the comments below.  The troubled code (unrolling and
> > renaming) was removed and replaced by a direct dependence computation
> > using df.c.  We also added support for loops with unknown bounds.
>
>   This version of patch is much better.  In whole the patch is
> practically ready to be committed.  But I still see some problems with
> the patch (most of them are minor).
>
> > 2004-04-15  Ayal Zaks  <zaks@il.ibm.com>
> >       Mostafa Hagog  <mustafa@il.ibm.com>
> >
> >       * Makefile.in (modulo-sched.o, ddg.o): New.
> >       * ddg.h, ddg.c, modulo-sched.c: New files.
> >       * cfglayout.c (duplicate_insn_chain): Remove "static" and push
> >       internals to "dupicate_insn".
> >       (duplicate_insn): New function.
> >       * cfglayout.h (duplicate_insn_chain, duplicate_insn): New
> > declarations.
> >       * common.opt (fmodulo-sched): New flag.
> >       * df.c (df_bb_regno_last_use_find, df_bb_regno_first_def_find):
> > Remove
> >       static and forward declaration.
> >       (df_find_def, df_reg_used, df_bb_regno_last_def_find) : New
> > functions.
> >       * df.h (df_bb_regno_last_use_find, df_bb_regno_first_def_find,
> >       df_bb_regno_last_def_find, df_find_def, df_reg_used): New
> > declarations.
> >       * flags.h (flag_modulo_sched): New flag.
> >       * opts.c (case OPT_fmodulo_sched): Handle new flag.
>                   ^ It should be the function name (common_handle_option)
> >       * params.def (max-sms-loop-number, sms-max-ii-factor,
> > sms-dfa-history,
> >       sms-loop-average-count-threshold): New parameters.
> >       * params.h (MAX-SMS-LOOP-NUMBER, SMS-MAX-II-FACTOR,
SMS_DFA_HISTORY,
>                        ^   ....     Undelines insteod of hyphen
> >       SMS_LOOP_AVERAGE_COUNT_THRESHOLD): New parameters.
> >       * passes.c ("sms", "sms-vcg"): New dumps.
> >       (rest_of_handle_sched): Call sms_schedule.
> >       * rtl.h (sms_schedule): New declaration.
> >       * timevar.def (TV_SMS): New.
> >       * toplev.c (flag_modulo_sched): Initialize.
> >       ("modulo-sched"): Handle -f option.
>          ^ Should be f_options
>          It should be
> >       * docs/invoke.texi: Document -fmodulo-sched option.
> >       * docs/passes.texi: Document new SMS pass.
>
>
>
> > static void
> > create_ddg_dep_no_link (ddg_ptr g, ddg_node_ptr from, ddg_node_ptr to,
> >          dep_type d_t, dep_data_type d_dt, int distance)
>
> There is no comment for this function.
>
> There are some typos could be fixed later.
>
> The biggest problem I see is functions recognizing with memory.  The
> following code is oriented to RISC architectures (not all).  It will
> not find insns with memory access for complicated cases.  It results
> in that you can miss memory dependencies (by the way the algorithm to
> find memory dependencies are quite conservative now but it could be
> improved later) and gcc will generate invalid code ore even crash in
> subsequent passes.
>
>
> > /* Returns non-zero if INSN reads from memory.  */
> > static int
> > mem_read_insn_p (rtx insn)
> > {
> >   rtx set = single_set (insn);
> >
> >   return (set && GET_CODE (SET_SRC (set)) == MEM);
> > }
> >
>
> The code could look like.
>
> static bool mem_ref_p;
>
> static void
> mark_mem_use (rtx *x, void *data)
> {
>   if (GET_CODE (*x) == MEM)
>     mem_ref_p = true;
> }
>
> static void
> mark_mem_use_1 (rtx *x, void *data)
> {
>   for_each_rtx (x, mark_mem_use, data);
> }
>
> static bool
> mem_read_insn_p (rtx insn)
> {
>   mem_ref_p = false;
>   note_uses (&PATTERN (insn), mark_mem_use_1, NULL);
>   return mem_ref_p;
> }
>
> > /* Returns non-zero if INSN writes to memory.  */
> > static int
> > mem_write_insn_p (rtx insn)
> > {
> >   rtx set = single_set (insn);
> >
> >   return (set && GET_CODE (SET_DEST (set)) == MEM);
> > }
> >
>
> The code could look like
>
> static void
> mark_mem_store (rtx loc, rtx setter, void *data)
> {
>   if (GET_CODE (loc) == MEM)
>     mem_ref_p = true;
> }
>
> static bool
> mem_write_insn_p (rtx insn)
> {
>   mem_ref_p = false;
>   note_stores (PATTERN (insn), mark_mem_store, NULL);
>   return mem_ref_p;
> }
>
> >
> > /* Returns non-zero if INSN reads to or writes from memory.  */
> > static int
> > mem_access_insn_p (rtx insn)
> > {
> >   rtx set = single_set (insn);
> >
> >   return (set
> >      && (GET_CODE (SET_SRC (set)) == MEM
> >          || GET_CODE (SET_DEST (set)) == MEM));
> > }
> >
> >
>
> The code could look like
>
> static bool
> rtx_mem_access_p (rtx x)
> {
>   int i, j;
>   const char *fmt;
>
>    if (x == 0)
>      return false;
>
>    if (GET_CODE (x) == MEM)
>      return true;
>
>   fmt = GET_RTX_FORMAT (code);
>   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
>     {
>       if (fmt[i] == 'e')
>    {
>      if (rtx_mem_access_p (XEXP (x, i)))
>             return true;
>         }
>       else if (fmt[i] == 'E')
>    for (j = 0; j < XVECLEN (x, i); j++)
>      {
>        if (rtx_mem_access_p (XVECEXP (x, i, j)))
>               return true;
>           }
>     }
>   return false;
> }
>
> static bool
> mem_access_insn_p (rtx insn)
> {
>
> return rtx_mem_access_p (PATTERN (insn));
>
> }
>
> Please fix them and test the patch again on bootstrap and regression.
After
> that you can commit it.
>
> Vlad
>

Attachment: gcc3.5.patch
Description: Binary data

Attachment: modulo-sched.c
Description: Binary data

Attachment: ddg.c
Description: Binary data

Attachment: ddg.h
Description: Binary data