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] Name unnamed passes and change duplicate pass names


Thanks for pointing me in the right direction.  It turns out that
naming unnamed passes does indeed create unnecessary dump files.
These pass files do not include dumps, however, only ;; Function
headers.  Sorry for the late response, by the way; it took me longer
than I expected to learn about the dump file system.

I think the best solution is to decouple the decision about whether a
pass should have a dump file from whether or not it is named (so we
can then name every pass).

I added the function needs_dump_file() to passes.c for this purpose:
it indicates that a pass needs a dump file if it requests a dump with
TODO_dump_func or TODO_dump_cgraph.  I also added a
TODO_require_dumpfile, which lets a pass indicate that it needs a dump
file without TODO_dump_func or TODO_dump_cgraph.

I'll provide an additional patch that adds TODO_require_dumpfile to
passes that need it.  I first thought that creating dump files for
passes with TODO_dump_func and TODO_dump_cgraph would be sufficient,
but it turns out that some passes without either of those flags output
other information to their dump files.

I did a before-and-after spot check to make sure dump output stays the
same.  There are a few places where I allowed the patched version's
behavior to deviate because it's probably more correct.

These passes already had names before I started but do not generate
any dump output that I could find:

early_optimizations
sibling
subregs_of_mode_init
subregs_of_mode_finish
dfinish

I assume that these dump files are superfluous, so I don't plan to
give them TODO_require_dumpfile flags.

Also, pass_referenced_vars previously did not generate a dump file (as
it was unnamed) but now does because it is marked TODO_dump_func.  I
assume that generating the dump is actually the desired behavior, so I
left that as it.

Finally, I got rid of all places that check if pass->name is NULL.  I
replaced these checks with a gcc_assert().  I can get rid of that if
it's too radical a change.

2009-05-12 Justin Seyster <jrseys@gmail.com>

gcc/
	* passes.c (register_dump_files_1, next_pass_1): Don't
	use (pass->name != NULL) to decide if a pass needs a dump file.
	(execute_one_pass): Remove special case for NULL pass->name.
	* tree-pass.h: Added TODO_require_dumpfile flag.
	* statistics.c (curr_statistics_hash): Added assert to make sure
	that a negative static_pasS_number never gets implicitly cast to
	an unsigned int.

Index: gcc/passes.c
===================================================================
--- gcc/passes.c	(revision 147136)
+++ gcc/passes.c	(working copy)
@@ -396,6 +396,26 @@
   set_pass_for_id (id, pass);
 }

+/* True if a set of TODO flags include any option that will output to
+   a dump file.  */
+
+static bool
+todo_includes_dump (unsigned int flags)
+{
+  return ((flags & TODO_dump_cgraph) || (flags & TODO_dump_func)
+          || (flags & TODO_require_dumpfile));
+}
+
+/* True if a pass needs a dump file because it wants to dump call
+   graphs or functions. */
+
+static bool
+needs_dump_file (struct opt_pass *pass)
+{
+  return (todo_includes_dump (pass->todo_flags_start)
+          || todo_includes_dump (pass->todo_flags_finish));
+}
+
 /* Recursive worker function for register_dump_files.  */

 static int
@@ -406,7 +426,8 @@
       int new_properties = (properties | pass->properties_provided)
 			   & ~pass->properties_destroyed;

-      if (pass->name && pass->name[0] != '*')
+      gcc_assert (pass->name);
+      if (needs_dump_file (pass) && pass->name[0] != '*')
         register_one_dump_file (pass);

       if (pass->sub)
@@ -459,7 +480,7 @@
          and so it should rename the dump file.  The first instance will
          be -1, and be number of duplicates = -static_pass_number - 1.
          Subsequent instances will be > 0 and just the duplicate number.  */
-      if (pass->name)
+      if (needs_dump_file (pass))
         {
           pass->static_pass_number -= 1;
           new_pass->static_pass_number = -pass->static_pass_number;
@@ -1261,8 +1282,9 @@
   if (pass->gate && !pass->gate ())
     return false;

+  gcc_assert (pass->name);
   if (!quiet_flag && !cfun)
-    fprintf (stderr, " <%s>", pass->name ? pass->name : "");
+    fprintf (stderr, " <%s>", pass->name);

   if (pass->todo_flags_start & TODO_set_props)
     cfun->curr_properties = pass->properties_required;
Index: gcc/tree-pass.h
===================================================================
--- gcc/tree-pass.h	(revision 147136)
+++ gcc/tree-pass.h	(working copy)
@@ -293,6 +293,10 @@
 /* Rebuild the addressable-vars bitmap and do register promotion.  */
 #define TODO_update_address_taken	(1 << 21)

+/* Dummy TODO that forces register_dump_files to create a dump file
+   even without TODO_dump_func or TODO_dump_cgraph.  */
+#define TODO_require_dumpfile		(1 << 22)
+
 #define TODO_update_ssa_any		\
     (TODO_update_ssa			\
      | TODO_update_ssa_no_phi		\
Index: gcc/statistics.c
===================================================================
--- gcc/statistics.c	(revision 147136)
+++ gcc/statistics.c	(working copy)
@@ -82,8 +82,11 @@
 static htab_t
 curr_statistics_hash (void)
 {
-  unsigned idx = current_pass->static_pass_number;
+  unsigned idx;

+  gcc_assert(current_pass->static_pass_number > 0);
+  idx = current_pass->static_pass_number;
+
   if (idx < nr_statistics_hashes
       && statistics_hashes[idx] != NULL)
     return statistics_hashes[idx];


On Fri, May 8, 2009 at 4:29 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
>
>> Does this cause dumps to happen even if there is nothing to dump?
>
> Yes, it does.
>
>> Maybe you need to add another field to the struct saying a dump should
>> be created.
>
> Or another TODO (since it should print the ";; Function" header in
> dump_file, having something like TODO_dump_header in todo_start makes
> sort of sense; and set dump_file == NULL if the TODO is not present so
> that having TODO_dump_func without TODO_dump_header will crash).
>
> Paolo
>


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