On Tue, Jun 26, 2018 at 10:27 PM David Malcolm <dmalc...@redhat.com> wrote:
>
> 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?
OK.
Thanks,
Richard.
> 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
>
