[PATCH] Hide alt_dump_file within dumpfile.c

Richard Biener richard.guenther@gmail.com
Thu Jun 28 09:50:00 GMT 2018


On Tue, Jun 26, 2018 at 10:27 PM David Malcolm <dmalcolm@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 <dmalcolm@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
>



More information about the Gcc-patches mailing list