This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/3] Fix -fopt-info for plugin passes


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)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]