Re: [PING][PATCH]: Improvements to sched-deps.c interface

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

Vladimir Makarov wrote:

...

> In general, the patch is ok.  Although I have minor comments (see below).

Vladimir, sorry for the delay with my reply - I've had little time last 
two weeks available for projects of my own.  Your comments have helped 
to make this patch better and below are hunks I've changed - please take 
a look at them and see if they are ok.

> It is very hard to read sched-deps.c patch, please next time check the
> patches and if they look like a mess then use context format
> instead unified.

Sorry for that, next time I'll be more considerate.

> I don't think that you need approval for rs6000.c patch.  It is so
> obvious (just renaming).  In any case, I CCed this email to rs6000
> maintainer David Edelhson if he wants to comment rs6000.c part (David,
> rs6000.c is at end of email).
>
> Please wait for approval from Jim Wilson for ia64.c (this part is at
> the end of email) and from Ayal Zacks for ddg.c (although ddg.c uses
> functions from the scheduler I believe is a part of modulo scheduler.
> Ayal, ddg.c part is right after ChangeLog.).  They looks ok for me but
> I am not a maintainer of these files.

The only part still unapproved is ia64 changes.  Jim?

> The patch is ok for me with the following comments fixed.

...

> Please, describe the constants.  Their meaning is pretty obvious for
> you and me but may be not for others.
>
> > +#define DUMP_DEP_PRO (2)
> > +#define DUMP_DEP_CON (4)
> > +#define DUMP_DEP_STATUS (8)
> > +
> > +#define DUMP_DEP_ALL (DUMP_DEP_PRO | DUMP_DEP_CON | DUMP_DEP_STATUS)
> > +
> > +/* Dump DEP to DUMP.
> > +   FLAGS is a bit mask specifying what information about DEP needs
> > +   to be printed.  If FLAGS has the very first bit set then dump
> > +   all fields of DEP.  */
>
> I don't understand why you need a special case with 1.  You can and
> should use DUMP_DEP_ALL and you should start the enumeration of the
> above constants with 1 (not 2).

I should have written better comments on this and now I did.  I've 
refactored that part, but left the special case with '1'.  Please see 
explanation after the hunk.  If you still don't like the '1' case, I'll 
remove it.

+/* Define flags for dump_dep ().  */
+
+/* Dump producer of the dependence.  */
+#define DUMP_DEP_PRO (2)
+
+/* Dump consumer of the dependence.  */
+#define DUMP_DEP_CON (4)
+
+/* Dump type of the dependence.  */
+#define DUMP_DEP_TYPE (8)
+
+/* Dump status of the dependence.  */
+#define DUMP_DEP_STATUS (16)
+
+/* Dump all information about the dependence.  */
+#define DUMP_DEP_ALL (DUMP_DEP_PRO | DUMP_DEP_CON | DUMP_DEP_TYPE	\
+		      |DUMP_DEP_STATUS)
+
+/* Dump DEP to DUMP.
+   FLAGS is a bit mask specifying what information about DEP needs
+   to be printed.
+   If FLAGS has the very first bit set, then dump all information about DEP
+   and propagate this bit into the callee dump functions.  */
+static void
+dump_dep (FILE *dump, dep_t dep, int flags)
 {
+  if (flags & 1)
+    flags |= DUMP_DEP_ALL;
+
+  fprintf (dump, "<");
+
+  if (flags & DUMP_DEP_PRO)
+    fprintf (dump, "%d; ", INSN_UID (DEP_PRO (dep)));
+
+  if (flags & DUMP_DEP_CON)
+    fprintf (dump, "%d; ", INSN_UID (DEP_CON (dep)));
+
+  if (flags & DUMP_DEP_TYPE)
+    {
+      char t;
+      enum reg_note type = DEP_TYPE (dep);
+
+      switch (type)
+	{
+	case REG_DEP_TRUE:
+	  t = 't';
+	  break;
+
+	case REG_DEP_OUTPUT:
+	  t = 'o';
+	  break;
+
+	case REG_DEP_ANTI:
+	  t = 'a';
+	  break;

+	default:
+	  gcc_unreachable ();
+	  break;
+	}
+
+      fprintf (dump, "%c; ", t);
+    }
+
+  if (flags & DUMP_DEP_STATUS)
+    {
+      if (current_sched_info->flags & USE_DEPS_LIST)
+	dump_ds (dump, DEP_STATUS (dep));
+    }
+
+  fprintf (dump, ">");
 }

+/* Default flags for dump_dep ().  */
+static int dump_dep_flags = (DUMP_DEP_PRO | DUMP_DEP_CON);
+
+/* Dump all fields of DEP to STDERR.  */
+void
+sd_debug_dep (dep_t dep)
+{
+  dump_dep (stderr, dep, 1);
+  fprintf (stderr, "\n");
+}

+
+/* Dump size of the lists.  */
+#define DUMP_LISTS_SIZE (2)
+
+/* Dump dependencies of the lists.  */
+#define DUMP_LISTS_DEPS (4)
+
+/* Dump all information about the lists.  */
+#define DUMP_LISTS_ALL (DUMP_LISTS_SIZE | DUMP_LISTS_DEPS)
+
+/* Dump deps_lists of INSN specified by TYPES to DUMP.
+   FLAGS is a bit mask specifying what information about the lists needs
+   to be printed.
+   If FLAGS has the very first bit set, then dump all information about
+   the lists and propagate this bit into the callee dump functions.  */
+static void
+dump_lists (FILE *dump, rtx insn, sd_list_types_def types, int flags)
+{
+  sd_iterator_def sd_it;
+  dep_t dep;
+  int all;
+
+  all = (flags & 1);
+
+  if (all)
+    flags |= DUMP_LISTS_ALL;
+
+  fprintf (dump, "[");
+
+  if (flags & DUMP_LISTS_SIZE)
+    fprintf (dump, "%d; ", sd_lists_size (insn, types));
+
+  if (flags & DUMP_LISTS_DEPS)
+    {
+      FOR_EACH_DEP (insn, types, sd_it, dep)
+	{
+	  dump_dep (dump, dep, dump_dep_flags | all);
+	  fprintf (dump, " ");
+	}
+    }
+}
+
+/* Dump all information about deps_lists of INSN specified by TYPES
+   to STDERR.  */
+void
+sd_debug_lists (rtx insn, sd_list_types_def types)
+{
+  dump_lists (stderr, insn, types, 1);
+  fprintf (stderr, "\n");
 }

The reason to use '1' is that it is propagated to the inner callees. 
Dumping functions have two main usecases: to dump info to the log and to 
be invoked within debugger.  The difference between the usecases is in 
the amount of information we wish to get: for the logs we want general 
info without details and in the debugger session we want every little 
detail to be printed to the console.  As both functions - dump_dep () 
and dump_lists () - have their own set of flags and one gets called 
within the other, there should be a convenient way to pass information 
through dump_lists () to dump_dep () about how much to dump.  That is 
what '1' handling does.

...

> > +extern bool haifa_recovery_block_was_added_during_scheduling_p;
>
> Personaly I like longer names because they behave like comments.  But 
> probably it is too long.  Another one is
>
> haifa_recovery_block_was_added_during_current_schedule_block_p
>
> Moreover the both have the same prefix of 38 chars long.  As I
> remember, ANSI C standard differ only first 31 chars for external
> identifiers.
>
> Please, make them shorter.

They are haifa_recovery_bb_recently_added_p and 
haifa_recovery_bb_ever_added_p now.

-----------------

I'm now retesting the patch on ppc64 (with -fmodulo-sched), x86_64 and 
ia64 to make sure the patch doesn't change schedulers behavior.


Thanks,

Maxim