| Message ID | 1530047468-37685-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hide alt_dump_file within dumpfile.c | expand |
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 >
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); } }