This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/3] Fix -fopt-info for plugin passes
- From: Richard Biener <rguenther at suse dot de>
- To: David Malcolm <dmalcolm at redhat dot com>
- Cc: Richard Sandiford <richard dot sandiford at arm dot com>, gcc-patches at gcc dot gnu dot org
- Date: Thu, 4 Oct 2018 12:39:14 +0200 (CEST)
- Subject: Re: [PATCH 1/3] Fix -fopt-info for plugin passes
- References: <87k1q17omd.fsf@arm.com> <1538161880-64793-1-git-send-email-dmalcolm@redhat.com> <1538161880-64793-2-git-send-email-dmalcolm@redhat.com>
On Fri, 28 Sep 2018, David Malcolm wrote:
> Attempts to dump via -fopt-info from a plugin pass fail, due
> to the dfi->alt_state for such passes never being set.
>
> This is because the -fopt-info options were being set up per-pass
> during option-parsing (via gcc::dump_manager::opt_info_enable_passes),
> but this data was not retained or used it for passes created later
> (for plugins and target-specific passes).
>
> This patch fixes the issue by storing the -fopt-info options into
> gcc::dump_manager, refactoring the dfi-setup code out of
> opt_info_enable_passes, and reusing it for such passes, fixing the
> issue. The patch adds a demo plugin to test that dumping from a
> plugin works.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
OK.
Richard.
> gcc/ChangeLog:
> * dumpfile.c (gcc::dump_manager::dump_manager): Initialize new
> fields.
> (gcc::dump_manager::~dump_manager): Free m_optinfo_filename.
> (gcc::dump_manager::register_pass): New member function, adapted
> from loop body in gcc::pass_manager::register_pass, adding a
> call to update_dfi_for_opt_info.
> (gcc::dump_manager::opt_info_enable_passes): Store the
> -fopt-info options into the new fields. Move the loop
> bodies into...
> (gcc::dump_manager::update_dfi_for_opt_info): ...this new member
> function.
> * dumpfile.h (struct opt_pass): New forward decl.
> (gcc::dump_manager::register_pass): New decl.
> (gcc::dump_manager::update_dfi_for_opt_info): New decl.
> (class gcc::dump_manager): Add fields "m_optgroup_flags",
> "m_optinfo_flags", and "m_optinfo_filename".
> * passes.c (gcc::pass_manager::register_pass): Move all of the
> dump-handling code to gcc::dump_manager::register_pass.
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/plugin/dump-1.c: New test.
> * gcc.dg/plugin/dump_plugin.c: New test plugin.
> * gcc.dg/plugin/plugin.exp (plugin_test_list): Add the above.
> ---
> gcc/dumpfile.c | 120 +++++++++++++++++--------
> gcc/dumpfile.h | 12 +++
> gcc/passes.c | 30 ++-----
> gcc/testsuite/gcc.dg/plugin/dump-1.c | 28 ++++++
> gcc/testsuite/gcc.dg/plugin/dump_plugin.c | 143 ++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/plugin/plugin.exp | 2 +
> 6 files changed, 275 insertions(+), 60 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/plugin/dump-1.c
> create mode 100644 gcc/testsuite/gcc.dg/plugin/dump_plugin.c
>
> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> index d430ea3..d359e41 100644
> --- a/gcc/dumpfile.c
> +++ b/gcc/dumpfile.c
> @@ -177,12 +177,16 @@ gcc::dump_manager::dump_manager ():
> m_next_dump (FIRST_AUTO_NUMBERED_DUMP),
> m_extra_dump_files (NULL),
> m_extra_dump_files_in_use (0),
> - m_extra_dump_files_alloced (0)
> + m_extra_dump_files_alloced (0),
> + m_optgroup_flags (OPTGROUP_NONE),
> + m_optinfo_flags (TDF_NONE),
> + m_optinfo_filename (NULL)
> {
> }
>
> gcc::dump_manager::~dump_manager ()
> {
> + free (m_optinfo_filename);
> for (size_t i = 0; i < m_extra_dump_files_in_use; i++)
> {
> dump_file_info *dfi = &m_extra_dump_files[i];
> @@ -1512,6 +1516,50 @@ dump_flag_name (int phase) const
> return dfi->swtch;
> }
>
> +/* Handle -fdump-* and -fopt-info for a pass added after
> + command-line options are parsed (those from plugins and
> + those from backends).
> +
> + Because the registration of plugin/backend passes happens after the
> + command-line options are parsed, the options that specify single
> + pass dumping (e.g. -fdump-tree-PASSNAME) cannot be used for new
> + passes. Therefore we currently can only enable dumping of
> + new passes when the 'dump-all' flags (e.g. -fdump-tree-all)
> + are specified. This is done here.
> +
> + Similarly, the saved -fopt-info options are wired up to the new pass. */
> +
> +void
> +gcc::dump_manager::register_pass (opt_pass *pass)
> +{
> + gcc_assert (pass);
> +
> + register_one_dump_file (pass);
> +
> + dump_file_info *pass_dfi = get_dump_file_info (pass->static_pass_number);
> + gcc_assert (pass_dfi);
> +
> + enum tree_dump_index tdi;
> + if (pass->type == SIMPLE_IPA_PASS
> + || pass->type == IPA_PASS)
> + tdi = TDI_ipa_all;
> + else if (pass->type == GIMPLE_PASS)
> + tdi = TDI_tree_all;
> + else
> + tdi = TDI_rtl_all;
> + const dump_file_info *tdi_dfi = get_dump_file_info (tdi);
> + gcc_assert (tdi_dfi);
> +
> + /* Check if dump-all flag is specified. */
> + if (tdi_dfi->pstate)
> + {
> + pass_dfi->pstate = tdi_dfi->pstate;
> + pass_dfi->pflags = tdi_dfi->pflags;
> + }
> +
> + update_dfi_for_opt_info (pass_dfi);
> +}
> +
> /* Finish a tree dump for PHASE. STREAM is the stream created by
> dump_begin. */
>
> @@ -1587,47 +1635,47 @@ opt_info_enable_passes (optgroup_flags_t optgroup_flags, dump_flags_t flags,
> const char *filename)
> {
> int n = 0;
> - size_t i;
>
> - for (i = TDI_none + 1; i < (size_t) TDI_end; i++)
> - {
> - if ((dump_files[i].optgroup_flags & optgroup_flags))
> - {
> - const char *old_filename = dump_files[i].alt_filename;
> - /* Since this file is shared among different passes, it
> - should be opened in append mode. */
> - dump_files[i].alt_state = 1;
> - dump_files[i].alt_flags |= flags;
> - n++;
> - /* Override the existing filename. */
> - if (filename)
> - dump_files[i].alt_filename = xstrdup (filename);
> - if (old_filename && filename != old_filename)
> - free (CONST_CAST (char *, old_filename));
> - }
> - }
> + m_optgroup_flags = optgroup_flags;
> + m_optinfo_flags = flags;
> + m_optinfo_filename = xstrdup (filename);
>
> - for (i = 0; i < m_extra_dump_files_in_use; i++)
> - {
> - if ((m_extra_dump_files[i].optgroup_flags & optgroup_flags))
> - {
> - const char *old_filename = m_extra_dump_files[i].alt_filename;
> - /* Since this file is shared among different passes, it
> - should be opened in append mode. */
> - m_extra_dump_files[i].alt_state = 1;
> - m_extra_dump_files[i].alt_flags |= flags;
> - n++;
> - /* Override the existing filename. */
> - if (filename)
> - m_extra_dump_files[i].alt_filename = xstrdup (filename);
> - if (old_filename && filename != old_filename)
> - free (CONST_CAST (char *, old_filename));
> - }
> - }
> + for (size_t i = TDI_none + 1; i < (size_t) TDI_end; i++)
> + if (update_dfi_for_opt_info (&dump_files[i]))
> + n++;
> +
> + for (size_t i = 0; i < m_extra_dump_files_in_use; i++)
> + if (update_dfi_for_opt_info (&m_extra_dump_files[i]))
> + n++;
>
> return n;
> }
>
> +/* Use the saved -fopt-info options to update DFI.
> + Return true if the dump is enabled. */
> +
> +bool
> +gcc::dump_manager::update_dfi_for_opt_info (dump_file_info *dfi) const
> +{
> + gcc_assert (dfi);
> +
> + if (!(dfi->optgroup_flags & m_optgroup_flags))
> + return false;
> +
> + const char *old_filename = dfi->alt_filename;
> + /* Since this file is shared among different passes, it
> + should be opened in append mode. */
> + dfi->alt_state = 1;
> + dfi->alt_flags |= m_optinfo_flags;
> + /* Override the existing filename. */
> + if (m_optinfo_filename)
> + dfi->alt_filename = xstrdup (m_optinfo_filename);
> + if (old_filename && m_optinfo_filename != old_filename)
> + free (CONST_CAST (char *, old_filename));
> +
> + return true;
> +}
> +
> /* Parse ARG as a dump switch. Return nonzero if it is, and store the
> relevant details in the dump_files array. */
>
> diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
> index 671b7b9..057ca46 100644
> --- a/gcc/dumpfile.h
> +++ b/gcc/dumpfile.h
> @@ -566,6 +566,8 @@ extern void dump_combine_total_stats (FILE *);
> /* In cfghooks.c */
> extern void dump_bb (FILE *, basic_block, int, dump_flags_t);
>
> +struct opt_pass;
> +
> namespace gcc {
>
> /* A class for managing all of the various dump files used by the
> @@ -634,6 +636,8 @@ public:
> const char *
> dump_flag_name (int phase) const;
>
> + void register_pass (opt_pass *pass);
> +
> private:
>
> int
> @@ -649,6 +653,8 @@ private:
> opt_info_enable_passes (optgroup_flags_t optgroup_flags, dump_flags_t flags,
> const char *filename);
>
> + bool update_dfi_for_opt_info (dump_file_info *dfi) const;
> +
> private:
>
> /* Dynamically registered dump files and switches. */
> @@ -657,6 +663,12 @@ private:
> size_t m_extra_dump_files_in_use;
> size_t m_extra_dump_files_alloced;
>
> + /* Stored values from -fopt-info, for handling passes created after
> + option-parsing (by backends and by plugins). */
> + optgroup_flags_t m_optgroup_flags;
> + dump_flags_t m_optinfo_flags;
> + char *m_optinfo_filename;
> +
> /* Grant access to dump_enable_all. */
> friend bool ::enable_rtl_dump_file (void);
>
> diff --git a/gcc/passes.c b/gcc/passes.c
> index 832f0b3..d838d90 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -1404,7 +1404,6 @@ void
> pass_manager::register_pass (struct register_pass_info *pass_info)
> {
> bool all_instances, success;
> - gcc::dump_manager *dumps = m_ctxt->get_dumps ();
>
> /* The checks below could fail in buggy plugins. Existing GCC
> passes should never fail these checks, so we mention plugin in
> @@ -1442,33 +1441,16 @@ pass_manager::register_pass (struct register_pass_info *pass_info)
>
> /* OK, we have successfully inserted the new pass. We need to register
> the dump files for the newly added pass and its duplicates (if any).
> - Because the registration of plugin/backend passes happens after the
> - command-line options are parsed, the options that specify single
> - pass dumping (e.g. -fdump-tree-PASSNAME) cannot be used for new
> - passes. Therefore we currently can only enable dumping of
> - new passes when the 'dump-all' flags (e.g. -fdump-tree-all)
> - are specified. While doing so, we also delete the pass_list_node
> + While doing so, we also delete the pass_list_node
> objects created during pass positioning. */
> + gcc::dump_manager *dumps = m_ctxt->get_dumps ();
> while (added_pass_nodes)
> {
> struct pass_list_node *next_node = added_pass_nodes->next;
> - enum tree_dump_index tdi;
> - register_one_dump_file (added_pass_nodes->pass);
> - if (added_pass_nodes->pass->type == SIMPLE_IPA_PASS
> - || added_pass_nodes->pass->type == IPA_PASS)
> - tdi = TDI_ipa_all;
> - else if (added_pass_nodes->pass->type == GIMPLE_PASS)
> - tdi = TDI_tree_all;
> - else
> - tdi = TDI_rtl_all;
> - /* Check if dump-all flag is specified. */
> - if (dumps->get_dump_file_info (tdi)->pstate)
> - {
> - dumps->get_dump_file_info (added_pass_nodes->pass->static_pass_number)
> - ->pstate = dumps->get_dump_file_info (tdi)->pstate;
> - dumps->get_dump_file_info (added_pass_nodes->pass->static_pass_number)
> - ->pflags = dumps->get_dump_file_info (tdi)->pflags;
> - }
> +
> + /* Handle -fdump-* and -fopt-info. */
> + dumps->register_pass (added_pass_nodes->pass);
> +
> XDELETE (added_pass_nodes);
> added_pass_nodes = next_node;
> }
> diff --git a/gcc/testsuite/gcc.dg/plugin/dump-1.c b/gcc/testsuite/gcc.dg/plugin/dump-1.c
> new file mode 100644
> index 0000000..165a9c1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/plugin/dump-1.c
> @@ -0,0 +1,28 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fopt-info-note" } */
> +
> +extern void test_string_literal (void);
> +extern void test_tree (void);
> +extern void test_gimple (int);
> +extern void test_cgraph_node (void);
> +extern void test_wide_int (void);
> +extern void test_poly_int (void);
> +extern void test_scopes (void);
> +
> +void test_remarks (void)
> +{
> + test_string_literal (); /* { dg-message "test of dump for 'test_string_literal'" } */
> + test_tree (); /* { dg-message "test of tree: 0" } */
> + test_gimple (42); /* { dg-message "test of gimple: test_gimple \\(42\\);" } */
> + test_cgraph_node (); /* { dg-message "test of callgraph node: test_cgraph_node/\[0-9\]+" } */
> + test_wide_int (); /* { dg-message "test of wide int: 0" } */
> + test_poly_int (); /* { dg-message "test of poly int: 42" } */
> +
> + test_scopes (); /* { dg-line test_scopes_line } */
> + /* { dg-message "=== outer scope ===" "" { target *-*-* } test_scopes_line } */
> + /* { dg-message " at outer scope" "" { target *-*-* } test_scopes_line } */
> + /* { dg-message " === middle scope ===" "" { target *-*-* } test_scopes_line } */
> + /* { dg-message " at middle scope" "" { target *-*-* } test_scopes_line } */
> + /* { dg-message " === innermost scope ===" "" { target *-*-* } test_scopes_line } */
> + /* { dg-message " at innermost scope" "" { target *-*-* } test_scopes_line } */
> +}
> diff --git a/gcc/testsuite/gcc.dg/plugin/dump_plugin.c b/gcc/testsuite/gcc.dg/plugin/dump_plugin.c
> new file mode 100644
> index 0000000..12573d6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/plugin/dump_plugin.c
> @@ -0,0 +1,143 @@
> +/* Plugin for testing dumpfile.c. */
> +
> +#include "gcc-plugin.h"
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tree.h"
> +#include "tree-pass.h"
> +#include "intl.h"
> +#include "plugin-version.h"
> +#include "diagnostic.h"
> +#include "context.h"
> +#include "optinfo.h"
> +#include "gimple.h"
> +#include "gimple-iterator.h"
> +#include "cgraph.h"
> +
> +int plugin_is_GPL_compatible;
> +
> +const pass_data pass_data_test_dumping =
> +{
> + GIMPLE_PASS, /* type */
> + "test_dumping", /* name */
> + OPTGROUP_LOOP, /* optinfo_flags */
> + TV_NONE, /* tv_id */
> + PROP_ssa, /* properties_required */
> + 0, /* properties_provided */
> + 0, /* properties_destroyed */
> + 0, /* todo_flags_start */
> + 0, /* todo_flags_finish */
> +};
> +
> +class pass_test_dumping : public gimple_opt_pass
> +{
> +public:
> + pass_test_dumping (gcc::context *ctxt)
> + : gimple_opt_pass (pass_data_test_dumping, ctxt)
> + {}
> +
> + /* opt_pass methods: */
> + bool gate (function *) { return true; }
> + virtual unsigned int execute (function *);
> +
> +}; // class pass_test_dumping
> +
> +unsigned int
> +pass_test_dumping::execute (function *fun)
> +{
> + basic_block bb;
> +
> + if (!dump_enabled_p ())
> + return 0;
> +
> + FOR_ALL_BB_FN (bb, fun)
> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
> + !gsi_end_p (gsi); gsi_next (&gsi))
> + {
> + gimple *stmt = gsi_stmt (gsi);
> + gcall *call = dyn_cast <gcall *> (stmt);
> + if (!call)
> + continue;
> + tree callee_decl = gimple_call_fndecl (call);
> + if (!callee_decl)
> + continue;
> + tree callee_name = DECL_NAME (callee_decl);
> + if (!callee_name)
> + continue;
> + const char *callee = IDENTIFIER_POINTER (callee_name);
> +
> + /* Various dumping tests, done at callsites,
> + controlled by the callee name. */
> + if (strcmp (callee, "test_string_literal") == 0)
> + dump_printf_loc (MSG_NOTE, stmt, "test of dump for %qs\n",
> + callee);
> + else if (strcmp (callee, "test_tree") == 0)
> + dump_printf_loc (MSG_NOTE, stmt, "test of tree: %T\n",
> + integer_zero_node);
> + else if (strcmp (callee, "test_gimple") == 0)
> + dump_printf_loc (MSG_NOTE, stmt, "test of gimple: %G", stmt);
> + else if (strcmp (callee, "test_cgraph_node") == 0)
> + {
> + dump_printf_loc (MSG_NOTE, stmt, "test of callgraph node: ");
> + dump_symtab_node (MSG_NOTE, cgraph_node::get (callee_decl));
> + dump_printf (MSG_NOTE, "\n");
> + }
> + else if (strcmp (callee, "test_wide_int") == 0)
> + {
> + HOST_WIDE_INT val = 0;
> + dump_printf_loc (MSG_NOTE, stmt,
> + "test of wide int: " HOST_WIDE_INT_PRINT_DEC "\n",
> + val);
> + }
> + else if (strcmp (callee, "test_poly_int") == 0)
> + {
> + dump_printf_loc (MSG_NOTE, stmt, "test of poly int: ");
> + dump_dec (MSG_NOTE, poly_int64 (42));
> + dump_printf (MSG_NOTE, "\n");
> + }
> + else if (strcmp (callee, "test_scopes") == 0)
> + {
> + AUTO_DUMP_SCOPE ("outer scope", stmt);
> + {
> + dump_printf_loc (MSG_NOTE, stmt, "at outer scope\n");
> + AUTO_DUMP_SCOPE ("middle scope", stmt);
> + {
> + dump_printf_loc (MSG_NOTE, stmt, "at middle scope\n");
> + AUTO_DUMP_SCOPE ("innermost scope", stmt);
> + dump_printf_loc (MSG_NOTE, stmt, "at innermost scope\n");
> + }
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static gimple_opt_pass *
> +make_pass_test_dumping (gcc::context *ctxt)
> +{
> + return new pass_test_dumping (ctxt);
> +}
> +
> +int
> +plugin_init (struct plugin_name_args *plugin_info,
> + struct plugin_gcc_version *version)
> +{
> + struct register_pass_info pass_info;
> + const char *plugin_name = plugin_info->base_name;
> + int argc = plugin_info->argc;
> + struct plugin_argument *argv = plugin_info->argv;
> +
> + if (!plugin_default_version_check (version, &gcc_version))
> + return 1;
> +
> + pass_info.pass = make_pass_test_dumping (g);
> + pass_info.reference_pass_name = "ssa";
> + pass_info.ref_pass_instance_number = 1;
> + pass_info.pos_op = PASS_POS_INSERT_AFTER;
> + register_callback (plugin_name, PLUGIN_PASS_MANAGER_SETUP, NULL,
> + &pass_info);
> +
> + return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/plugin/plugin.exp b/gcc/testsuite/gcc.dg/plugin/plugin.exp
> index 46246a2..50db3ae 100644
> --- a/gcc/testsuite/gcc.dg/plugin/plugin.exp
> +++ b/gcc/testsuite/gcc.dg/plugin/plugin.exp
> @@ -101,6 +101,8 @@ set plugin_test_list [list \
> must-tail-call-2.c } \
> { expensive_selftests_plugin.c \
> expensive-selftests-1.c } \
> + { dump_plugin.c \
> + dump-1.c } \
> ]
>
> foreach plugin_test $plugin_test_list {
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)