On Mon, 2018-06-25 at 15:34 +0200, Richard Biener wrote:
> On Wed, Jun 20, 2018 at 6:34 PM David Malcolm <dmalc...@redhat.com>
> wrote:
> >
> > Here's v3 of the patch (one big patch this time, rather than a
> > kit).
> >
> > Like the v2 patch kit, this patch reuses the existing dump API,
> > rather than inventing its own.
> >
> > Specifically, it uses the dump_* functions in dumpfile.h that don't
> > take a FILE *, the ones that implicitly write to dump_file and/or
> > alt_dump_file. I needed a name for them, so I've taken to calling
> > them the "structured dump API" (better name ideas welcome).
> >
> > v3 eliminates v2's optinfo_guard class, instead using "dump_*_loc"
> > calls as delimiters when consolidating "dump_*" calls. There's a
> > new dump_context class which has responsibility for consolidating
> > them into optimization records.
> >
> > The dump_*_loc calls now capture more than just a location_t: they
> > capture the profile_count and the location in GCC's own sources
> > where
> > the dump is being emitted from.
> >
> > This works by introducing a new "dump_location_t" class as the
> > argument of those dump_*_loc calls. The dump_location_t can
> > be constructed from a gimple * or from an rtx_insn *, so that
> > rather than writing:
> >
> > dump_printf_loc (MSG_NOTE, gimple_location (stmt),
> > "some message: %i", 42);
> >
> > you can write:
> >
> > dump_printf_loc (MSG_NOTE, stmt,
> > "some message: %i", 42);
> >
> > and the dump_location_t constructor will grab the location_t and
> > profile_count of stmt, and the location of the "dump_printf_loc"
> > callsite (and gracefully handle "stmt" being NULL).
> >
> > Earlier versions of the patch captured the location of the
> > dump_*_loc call via preprocessor hacks, or didn't work properly;
> > this version of the patch works more cleanly: internally,
> > dump_location_t is split into two new classes:
> > * dump_user_location_t: the location_t and profile_count within
> > the *user's code*, and
> > * dump_impl_location_t: the __builtin_FILE/LINE/FUNCTION within
> > the *implementation* code (i.e. GCC or a plugin), captured
> > "automagically" via default params
> >
> > These classes are sometimes used elsewhere in the code. For
> > example, "vect_location" becomes a dump_user_location_t
> > (location_t and profile_count), so that in e.g:
> >
> > vect_location = find_loop_location (loop);
> >
> > it's capturing the location_t and profile_count, and then when
> > it's used here:
> >
> > dump_printf_loc (MSG_NOTE, vect_location, "foo");
> >
> > the dump_location_t is constructed from the vect_location
> > plus the dump_impl_location_t at that callsite.
> >
> > In contrast, loop-unroll.c's report_unroll's "locus" param
> > becomes a dump_location_t: we're interested in where it was
> > called from, not in the locations of the various dump_*_loc calls
> > within it.
> >
> > Previous versions of the patch captured a gimple *, and needed
> > GTY markers; in this patch, the dump_user_location_t is now just a
> > location_t and a profile_count.
> >
> > The v2 patch added an overload for dump_printf_loc so that you
> > could pass in either a location_t, or the new type; this version
> > of the patch eliminates that: they all now take dump_location_t.
> >
> > Doing so required adding support for rtx_insn *, so that one can
> > write this kind of thing in RTL passes:
> >
> > dump_printf_loc (MSG_NOTE, insn, "foo");
> >
> > One knock-on effect is that get_loop_location now returns a
> > dump_user_location_t rather than a location_t, so that it has
> > hotness information.
> >
> > Richi: would you like me to split out this location-handling
> > code into a separate patch? (It's kind of redundant without
> > adding the remarks and optimization records work, but if that's
> > easier I can do it)
>
> I think that would be easier because it doesn't require the JSON
> stuff and so I'll happily approve it.
>
> Thus - trying to review that bits (and sorry for the delay).
>
> + location_t srcloc = loc.get_location_t ();
> +
> if (dump_file && (dump_kind & pflags))
> {
> - dump_loc (dump_kind, dump_file, loc);
> + dump_loc (dump_kind, dump_file, srcloc);
> print_gimple_stmt (dump_file, gs, spc, dump_flags |
> extra_dump_flags);
> }
>
> if (alt_dump_file && (dump_kind & alt_flags))
> {
> - dump_loc (dump_kind, alt_dump_file, loc);
> + dump_loc (dump_kind, alt_dump_file, srcloc);
> print_gimple_stmt (alt_dump_file, gs, spc, dump_flags |
> extra_dump_flags);
> }
> +
> + if (optinfo_enabled_p ())
> + {
> + optinfo &info = begin_next_optinfo (loc);
> + info.handle_dump_file_kind (dump_kind);
> + info.add_stmt (gs, extra_dump_flags);
> + }
>
> seeing this in multiple places. I seem to remember that
> dump_file / alt_dump_file was suposed to handle dumping
> into two locations - a dump file and optinfo (or stdout). This looks
> like the optinfo "stream" is even more separate. Could that
> obsolete the alt_dump_file stream? I'd need to review existing stuff
> in more detail to answer but maybe you already know from recently
> digging into this.
[...snip...]
Although I haven't yet obsoleted the alt_dump_file stream via the
optinfo "stream", I think it's possible. The following patch would be
necessary (and it helps with the rest of the optinfo work).
This patch removes alt_dump_file from dumpfile.h, making it static
within dumpfile.c. This allows for changing how -fopt-info is
implemented, and potentially adding other kinds of dump target, such
as remarks or optimization records.
Doing so requires changing the implementation of dump_enabled_p, so
the patch changes this to a simple lookup of a boolean global, which
is updated any time dump_file or alt_dump_file change.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
OK for trunk?
Thanks
Dave
gcc/ChangeLog:
* cgraph.c (cgraph_node::get_body): Replace assignments to
"dump_file" with calls to set_dump_file.
* dumpfile.c (alt_dump_file): Make static, and group with...
(alt_flags): ...this definition.
(dumps_are_enabled): New variable.
(refresh_dumps_are_enabled): New function.
(set_dump_file): New function.
(set_alt_dump_file): New function.
(gcc::dump_manager::dump_start): Replace assignments to
"dump_file" and "alt_dump_file" with calls to set_dump_file and
set_alt_dump_file.
(gcc::dump_manager::dump_finish): Likewise.
* dumpfile.h (alt_dump_file): Delete decl.
(dumps_are_enabled): New variable decl.
(set_dump_file): New function decl.
(dump_enabled_p): Rewrite in terms of new "dumps_are_enabled"
global.
* tree-nested.c (lower_nested_functions): Replace assignments to
"dump_file" with calls to set_dump_file.
---
gcc/cgraph.c | 4 ++--
gcc/dumpfile.c | 46 ++++++++++++++++++++++++++++++++++++++++------
gcc/dumpfile.h | 7 +++++--
gcc/tree-nested.c | 4 ++--
4 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 3899467..d19f1aa 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -3582,7 +3582,7 @@ cgraph_node::get_body (void)
const char *saved_dump_file_name = dump_file_name;
dump_flags_t saved_dump_flags = dump_flags;
dump_file_name = NULL;
- dump_file = NULL;
+ set_dump_file (NULL);
push_cfun (DECL_STRUCT_FUNCTION (decl));
execute_all_ipa_transforms ();
@@ -3593,7 +3593,7 @@ cgraph_node::get_body (void)
updated = true;
current_pass = saved_current_pass;
- dump_file = saved_dump_file;
+ set_dump_file (saved_dump_file);
dump_file_name = saved_dump_file_name;
dump_flags = saved_dump_flags;
}
diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 190b52d..458d1d7 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -40,18 +40,52 @@ along with GCC; see the file COPYING3. If not see
(strncmp (whole, part, strlen (part)) ? NULL : whole + strlen (part))
static dump_flags_t pflags; /* current dump_flags */
-static dump_flags_t alt_flags; /* current opt_info flags */
static void dump_loc (dump_flags_t, FILE *, source_location);
+
+/* Current -fopt-info output stream, if any, and flags. */
+static FILE *alt_dump_file = NULL;
+static dump_flags_t alt_flags;
+
static FILE *dump_open_alternate_stream (struct dump_file_info *);
/* These are currently used for communicating between passes.
However, instead of accessing them directly, the passes can use
dump_printf () for dumps. */
FILE *dump_file = NULL;
-FILE *alt_dump_file = NULL;
const char *dump_file_name;
dump_flags_t dump_flags;
+bool dumps_are_enabled = false;
+
+
+/* Update the "dumps_are_enabled" global; to be called whenever dump_file
+ or alt_dump_file change. */
+
+static void
+refresh_dumps_are_enabled ()
+{
+ dumps_are_enabled = (dump_file || alt_dump_file);
+}
+
+/* Set global "dump_file" to NEW_DUMP_FILE, refreshing the "dumps_are_enabled"
+ global. */
+
+void
+set_dump_file (FILE *new_dump_file)
+{
+ dump_file = new_dump_file;
+ refresh_dumps_are_enabled ();
+}
+
+/* Set "alt_dump_file" to NEW_ALT_DUMP_FILE, refreshing the "dumps_are_enabled"
+ global. */
+
+static void
+set_alt_dump_file (FILE *new_alt_dump_file)
+{
+ alt_dump_file = new_alt_dump_file;
+ refresh_dumps_are_enabled ();
+}
#define DUMP_FILE_INFO(suffix, swtch, dkind, num) \
{suffix, swtch, NULL, NULL, NULL, NULL, NULL, dkind, TDF_NONE, TDF_NONE, \
@@ -603,7 +637,7 @@ dump_start (int phase, dump_flags_t *flag_ptr)
}
free (name);
dfi->pstream = stream;
- dump_file = dfi->pstream;
+ set_dump_file (dfi->pstream);
/* Initialize current dump flags. */
pflags = dfi->pflags;
}
@@ -613,7 +647,7 @@ dump_start (int phase, dump_flags_t *flag_ptr)
{
dfi->alt_stream = stream;
count++;
- alt_dump_file = dfi->alt_stream;
+ set_alt_dump_file (dfi->alt_stream);
/* Initialize current -fopt-info flags. */
alt_flags = dfi->alt_flags;
}
@@ -644,8 +678,8 @@ dump_finish (int phase)
dfi->alt_stream = NULL;
dfi->pstream = NULL;
- dump_file = NULL;
- alt_dump_file = NULL;
+ set_dump_file (NULL);
+ set_alt_dump_file (NULL);
dump_flags = TDF_NONE;
alt_flags = TDF_NONE;
pflags = TDF_NONE;
diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
index 89d5c11..9828a3f 100644
--- a/gcc/dumpfile.h
+++ b/gcc/dumpfile.h
@@ -445,15 +445,18 @@ extern void dump_bb (FILE *, basic_block, int,
dump_flags_t);
/* Global variables used to communicate with passes. */
extern FILE *dump_file;
-extern FILE *alt_dump_file;
extern dump_flags_t dump_flags;
extern const char *dump_file_name;
+extern bool dumps_are_enabled;
+
+extern void set_dump_file (FILE *new_dump_file);
+
/* Return true if any of the dumps is enabled, false otherwise. */
static inline bool
dump_enabled_p (void)
{
- return (dump_file || alt_dump_file);
+ return dumps_are_enabled;
}
/* Managing nested scopes, so that dumps can express the call chain
diff --git a/gcc/tree-nested.c b/gcc/tree-nested.c
index 127a81f..4c8eda9 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -3399,7 +3399,7 @@ lower_nested_functions (tree fndecl)
gimplify_all_functions (cgn);
- dump_file = dump_begin (TDI_nested, &dump_flags);
+ set_dump_file (dump_begin (TDI_nested, &dump_flags));
if (dump_file)
fprintf (dump_file, "\n;; Function %s\n\n",
lang_hooks.decl_printable_name (fndecl, 2));
@@ -3426,7 +3426,7 @@ lower_nested_functions (tree fndecl)
if (dump_file)
{
dump_end (TDI_nested, dump_file);
- dump_file = NULL;
+ set_dump_file (NULL);
}
}
--
1.8.5.3
