Re: [PATCH] Implementing Swing Modulo Scheduling in GCC

[Vladimir Makarov <vmakarov at redhat dot com>]

> 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