Add debug (slp_tree) and dump infrastructure for this

This adds an alternate debug_dump_context similar to the one for
selftests but for interactive debugging routines.  This allows
to share code between user-visible dumping via the dump_* API
and those debugging routines.  The primary driver was SLP node
dumping which wasn't accessible from inside a gdb session up to
now.

Bootstrap & regtest running on x86_64-unknown-linux-gnu.

David, does this look OK?

Thanks,
Richard.

2020-05-25  Richard Biener  <rguenther@suse.de>

	* dump-context.h (debug_dump_context): New class.
	(dump_context): Make it friend.
	* dumpfile.c (debug_dump_context::debug_dump_context):
	Implement.
	(debug_dump_context::~debug_dump_context): Likewise.
	* tree-vect-slp.c: Include dump-context.h.
	(vect_print_slp_tree): Dump a single SLP node.
	(debug): New overload for slp_tree.
	(vect_print_slp_graph): Rename from vect_print_slp_tree and
	use that.
	(vect_analyze_slp_instance): Adjust.
---
 gcc/dump-context.h  | 19 ++++++++++++++++++
 gcc/dumpfile.c      | 26 +++++++++++++++++++++++++
 gcc/tree-vect-slp.c | 47 +++++++++++++++++++++++++++++++++------------
 3 files changed, 80 insertions(+), 12 deletions(-)

On Mon, 2020-05-25 at 16:56 +0200, Richard Biener wrote:
> This adds an alternate debug_dump_context similar to the one for
> selftests but for interactive debugging routines.  This allows
> to share code between user-visible dumping via the dump_* API
> and those debugging routines.  The primary driver was SLP node
> dumping which wasn't accessible from inside a gdb session up to
> now.
> 
> Bootstrap & regtest running on x86_64-unknown-linux-gnu.
> 
> David, does this look OK?

Overall, seems sane to me; a couple of items inline below.

> Thanks,
> Richard.
> 
> 2020-05-25  Richard Biener  <rguenther@suse.de>
> 
> 	* dump-context.h (debug_dump_context): New class.
> 	(dump_context): Make it friend.
> 	* dumpfile.c (debug_dump_context::debug_dump_context):
> 	Implement.
> 	(debug_dump_context::~debug_dump_context): Likewise.
> 	* tree-vect-slp.c: Include dump-context.h.
> 	(vect_print_slp_tree): Dump a single SLP node.
> 	(debug): New overload for slp_tree.
> 	(vect_print_slp_graph): Rename from vect_print_slp_tree and
> 	use that.
> 	(vect_analyze_slp_instance): Adjust.
> ---
>  gcc/dump-context.h  | 19 ++++++++++++++++++
>  gcc/dumpfile.c      | 26 +++++++++++++++++++++++++
>  gcc/tree-vect-slp.c | 47 +++++++++++++++++++++++++++++++++--------
> ----
>  3 files changed, 80 insertions(+), 12 deletions(-)
> 
> diff --git a/gcc/dump-context.h b/gcc/dump-context.h
> index 347477f331e..6d51eaf31ad 100644
> --- a/gcc/dump-context.h
> +++ b/gcc/dump-context.h
> @@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  If not see
>  
>  class optrecord_json_writer;
>  namespace selftest { class temp_dump_context; }
> +class debug_dump_context;
>  
>  /* A class for handling the various dump_* calls.
>  
> @@ -42,6 +43,7 @@ namespace selftest { class temp_dump_context; }
>  class dump_context
>  {
>    friend class selftest::temp_dump_context;
> +  friend class debug_dump_context;
>  
>   public:
>    static dump_context &get () { return *s_current; }
> @@ -195,6 +197,23 @@ private:
>    auto_vec<stashed_item> m_stashed_items;
>  };
>  
> +/* An RAII-style class for use in debug dumpers for temporarily
> using a
> +   different dump_context.  */
> +
> +class debug_dump_context

(Bikeshed Alert)  The name might be confusing in that this class isn't
a dump_context itself.  Some of our existing RAII classes have an
"auto_" prefix; would that be an idea?
Maybe "auto_dump_everything"???

But I don't have a strong objection to the name as-is.

[...snip...]


> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> index 54718784fd4..0c0c076d890 100644
> --- a/gcc/dumpfile.c
> +++ b/gcc/dumpfile.c
> @@ -2078,6 +2078,32 @@ enable_rtl_dump_file (void)
>    return num_enabled > 0;
>  }
>  
> +/* debug_dump_context's ctor.  Temporarily override the dump_context
> +   (to forcibly enable output to stderr).  */
> +
> +debug_dump_context::debug_dump_context ()
> +: m_context (),
> +  m_saved (&dump_context::get ()),
> +  m_saved_flags (dump_flags),
> +  m_saved_file (dump_file)
> +{
> +  set_dump_file (stderr);
> +  dump_context::s_current = &m_context;
> +  pflags = dump_flags = MSG_ALL_KINDS | MSG_ALL_PRIORITIES;
> +  dump_context::get ().refresh_dumps_are_enabled ();
> +}
> +
> +/* debug_dump_context's dtor.  Restore the saved dump_context.  */
> +
> +debug_dump_context::~debug_dump_context ()
> +{
> +  set_dump_file (m_saved_file);
> +  dump_context::s_current = m_saved;
> +  pflags = dump_flags = m_saved_flags;
> +  dump_context::get ().refresh_dumps_are_enabled ();
> +}

I notice that the code saves dump_flags, and later restores both
dump_flags and pflags to the same value.  I'm a little hazy on this,
but is there any guarantee they had the same value?  Should the value
of pflags be saved separately from dump_flags?

[...snip...]


Hope this is constructive
Dave

On Tue, 26 May 2020, David Malcolm wrote:

> On Mon, 2020-05-25 at 16:56 +0200, Richard Biener wrote:
> > This adds an alternate debug_dump_context similar to the one for
> > selftests but for interactive debugging routines.  This allows
> > to share code between user-visible dumping via the dump_* API
> > and those debugging routines.  The primary driver was SLP node
> > dumping which wasn't accessible from inside a gdb session up to
> > now.
> > 
> > Bootstrap & regtest running on x86_64-unknown-linux-gnu.
> > 
> > David, does this look OK?
> 
> Overall, seems sane to me; a couple of items inline below.
> 
> > Thanks,
> > Richard.
> > 
> > 2020-05-25  Richard Biener  <rguenther@suse.de>
> > 
> > 	* dump-context.h (debug_dump_context): New class.
> > 	(dump_context): Make it friend.
> > 	* dumpfile.c (debug_dump_context::debug_dump_context):
> > 	Implement.
> > 	(debug_dump_context::~debug_dump_context): Likewise.
> > 	* tree-vect-slp.c: Include dump-context.h.
> > 	(vect_print_slp_tree): Dump a single SLP node.
> > 	(debug): New overload for slp_tree.
> > 	(vect_print_slp_graph): Rename from vect_print_slp_tree and
> > 	use that.
> > 	(vect_analyze_slp_instance): Adjust.
> > ---
> >  gcc/dump-context.h  | 19 ++++++++++++++++++
> >  gcc/dumpfile.c      | 26 +++++++++++++++++++++++++
> >  gcc/tree-vect-slp.c | 47 +++++++++++++++++++++++++++++++++--------
> > ----
> >  3 files changed, 80 insertions(+), 12 deletions(-)
> > 
> > diff --git a/gcc/dump-context.h b/gcc/dump-context.h
> > index 347477f331e..6d51eaf31ad 100644
> > --- a/gcc/dump-context.h
> > +++ b/gcc/dump-context.h
> > @@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  If not see
> >  
> >  class optrecord_json_writer;
> >  namespace selftest { class temp_dump_context; }
> > +class debug_dump_context;
> >  
> >  /* A class for handling the various dump_* calls.
> >  
> > @@ -42,6 +43,7 @@ namespace selftest { class temp_dump_context; }
> >  class dump_context
> >  {
> >    friend class selftest::temp_dump_context;
> > +  friend class debug_dump_context;
> >  
> >   public:
> >    static dump_context &get () { return *s_current; }
> > @@ -195,6 +197,23 @@ private:
> >    auto_vec<stashed_item> m_stashed_items;
> >  };
> >  
> > +/* An RAII-style class for use in debug dumpers for temporarily
> > using a
> > +   different dump_context.  */
> > +
> > +class debug_dump_context
> 
> (Bikeshed Alert)  The name might be confusing in that this class isn't
> a dump_context itself.  Some of our existing RAII classes have an
> "auto_" prefix; would that be an idea?
> Maybe "auto_dump_everything"???
> 
> But I don't have a strong objection to the name as-is.

kept it as-is but improved the class comment.

> [...snip...]
> 
> 
> > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> > index 54718784fd4..0c0c076d890 100644
> > --- a/gcc/dumpfile.c
> > +++ b/gcc/dumpfile.c
> > @@ -2078,6 +2078,32 @@ enable_rtl_dump_file (void)
> >    return num_enabled > 0;
> >  }
> >  
> > +/* debug_dump_context's ctor.  Temporarily override the dump_context
> > +   (to forcibly enable output to stderr).  */
> > +
> > +debug_dump_context::debug_dump_context ()
> > +: m_context (),
> > +  m_saved (&dump_context::get ()),
> > +  m_saved_flags (dump_flags),
> > +  m_saved_file (dump_file)
> > +{
> > +  set_dump_file (stderr);
> > +  dump_context::s_current = &m_context;
> > +  pflags = dump_flags = MSG_ALL_KINDS | MSG_ALL_PRIORITIES;
> > +  dump_context::get ().refresh_dumps_are_enabled ();
> > +}
> > +
> > +/* debug_dump_context's dtor.  Restore the saved dump_context.  */
> > +
> > +debug_dump_context::~debug_dump_context ()
> > +{
> > +  set_dump_file (m_saved_file);
> > +  dump_context::s_current = m_saved;
> > +  pflags = dump_flags = m_saved_flags;
> > +  dump_context::get ().refresh_dumps_are_enabled ();
> > +}
> 
> I notice that the code saves dump_flags, and later restores both
> dump_flags and pflags to the same value.  I'm a little hazy on this,
> but is there any guarantee they had the same value?  Should the value
> of pflags be saved separately from dump_flags?

Hum, right.  Better be safe than sorry.

Re-testing the following, will commit after that succeeds.

Richard.

From 73ddc25a76088571675aeccdd0537ebb2831b863 Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Mon, 25 May 2020 16:10:12 +0200
Subject: [PATCH] Add debug (slp_tree) and dump infrastructure for this

This adds an alternate debug_dump_context similar to the one for
selftests but for interactive debugging routines.  This allows
to share code between user-visible dumping via the dump_* API
and those debugging routines.  The primary driver was SLP node
dumping which wasn't accessible from inside a gdb session up to
now.

2020-05-27  Richard Biener  <rguenther@suse.de>

	* dump-context.h (debug_dump_context): New class.
	(dump_context): Make it friend.
	* dumpfile.c (debug_dump_context::debug_dump_context):
	Implement.
	(debug_dump_context::~debug_dump_context): Likewise.
	* tree-vect-slp.c: Include dump-context.h.
	(vect_print_slp_tree): Dump a single SLP node.
	(debug): New overload for slp_tree.
	(vect_print_slp_graph): Rename from vect_print_slp_tree and
	use that.
	(vect_analyze_slp_instance): Adjust.
---
 gcc/dump-context.h  | 21 ++++++++++++++++++++
 gcc/dumpfile.c      | 28 +++++++++++++++++++++++++++
 gcc/tree-vect-slp.c | 47 +++++++++++++++++++++++++++++++++------------
 3 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/gcc/dump-context.h b/gcc/dump-context.h
index 347477f331e..3f72cc932a9 100644
--- a/gcc/dump-context.h
+++ b/gcc/dump-context.h
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  If not see
 
 class optrecord_json_writer;
 namespace selftest { class temp_dump_context; }
+class debug_dump_context;
 
 /* A class for handling the various dump_* calls.
 
@@ -42,6 +43,7 @@ namespace selftest { class temp_dump_context; }
 class dump_context
 {
   friend class selftest::temp_dump_context;
+  friend class debug_dump_context;
 
  public:
   static dump_context &get () { return *s_current; }
@@ -195,6 +197,25 @@ private:
   auto_vec<stashed_item> m_stashed_items;
 };
 
+/* An RAII-style class for use in debug dumpers for temporarily using a
+   different dump_context.  It enables full details and outputs to
+   stderr instead of the currently active dump_file.  */
+
+class debug_dump_context
+{
+ public:
+  debug_dump_context ();
+  ~debug_dump_context ();
+
+ private:
+  dump_context m_context;
+  dump_context *m_saved;
+  dump_flags_t m_saved_flags;
+  dump_flags_t m_saved_pflags;
+  FILE *m_saved_file;
+};
+
+
 #if CHECKING_P
 
 namespace selftest {
diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 54718784fd4..5d61946fc49 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -2078,6 +2078,34 @@ enable_rtl_dump_file (void)
   return num_enabled > 0;
 }
 
+/* debug_dump_context's ctor.  Temporarily override the dump_context
+   (to forcibly enable output to stderr).  */
+
+debug_dump_context::debug_dump_context ()
+: m_context (),
+  m_saved (&dump_context::get ()),
+  m_saved_flags (dump_flags),
+  m_saved_pflags (pflags),
+  m_saved_file (dump_file)
+{
+  set_dump_file (stderr);
+  dump_context::s_current = &m_context;
+  pflags = dump_flags = MSG_ALL_KINDS | MSG_ALL_PRIORITIES;
+  dump_context::get ().refresh_dumps_are_enabled ();
+}
+
+/* debug_dump_context's dtor.  Restore the saved dump_context.  */
+
+debug_dump_context::~debug_dump_context ()
+{
+  set_dump_file (m_saved_file);
+  dump_context::s_current = m_saved;
+  dump_flags = m_saved_flags;
+  pflags = m_saved_pflags;
+  dump_context::get ().refresh_dumps_are_enabled ();
+}
+
+
 #if CHECKING_P
 
 namespace selftest {
diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
index c4fd045e9be..c0c9afd0bd2 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -44,6 +44,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "vec-perm-indices.h"
 #include "gimple-fold.h"
 #include "internal-fn.h"
+#include "dump-context.h"
 
 
 /* Initialize a SLP node.  */
@@ -1579,20 +1580,17 @@ fail:
   return node;
 }
 
-/* Dump a slp tree NODE using flags specified in DUMP_KIND.  */
+/* Dump a single SLP tree NODE.  */
 
 static void
 vect_print_slp_tree (dump_flags_t dump_kind, dump_location_t loc,
-		     slp_tree node, hash_set<slp_tree> &visited)
+		     slp_tree node)
 {
   unsigned i, j;
-  stmt_vec_info stmt_info;
   slp_tree child;
+  stmt_vec_info stmt_info;
   tree op;
 
-  if (visited.add (node))
-    return;
-
   dump_metadata_t metadata (dump_kind, loc.get_impl_location ());
   dump_user_location_t user_loc = loc.get_user_location ();
   dump_printf_loc (metadata, user_loc, "node%s %p (max_nunits=%u, refcnt=%u)\n",
@@ -1626,16 +1624,41 @@ vect_print_slp_tree (dump_flags_t dump_kind, dump_location_t loc,
   FOR_EACH_VEC_ELT (SLP_TREE_CHILDREN (node), i, child)
     dump_printf (dump_kind, " %p", (void *)child);
   dump_printf (dump_kind, "\n");
+}
+
+DEBUG_FUNCTION void
+debug (slp_tree node)
+{
+  debug_dump_context ctx;
+  vect_print_slp_tree (MSG_NOTE,
+		       dump_location_t::from_location_t (UNKNOWN_LOCATION),
+		       node);
+}
+
+/* Dump a slp tree NODE using flags specified in DUMP_KIND.  */
+
+static void
+vect_print_slp_graph (dump_flags_t dump_kind, dump_location_t loc,
+		      slp_tree node, hash_set<slp_tree> &visited)
+{
+  unsigned i;
+  slp_tree child;
+
+  if (visited.add (node))
+    return;
+
+  vect_print_slp_tree (dump_kind, loc, node);
+
   FOR_EACH_VEC_ELT (SLP_TREE_CHILDREN (node), i, child)
-    vect_print_slp_tree (dump_kind, loc, child, visited);
+    vect_print_slp_graph (dump_kind, loc, child, visited);
 }
 
 static void
-vect_print_slp_tree (dump_flags_t dump_kind, dump_location_t loc,
-		     slp_tree node)
+vect_print_slp_graph (dump_flags_t dump_kind, dump_location_t loc,
+		      slp_tree entry)
 {
   hash_set<slp_tree> visited;
-  vect_print_slp_tree (dump_kind, loc, node, visited);
+  vect_print_slp_graph (dump_kind, loc, entry, visited);
 }
 
 /* Mark the tree rooted at NODE with PURE_SLP.  */
@@ -2240,8 +2263,8 @@ vect_analyze_slp_instance (vec_info *vinfo,
 	    {
 	      dump_printf_loc (MSG_NOTE, vect_location,
 			       "Final SLP tree for instance:\n");
-	      vect_print_slp_tree (MSG_NOTE, vect_location,
-				   SLP_INSTANCE_TREE (new_instance));
+	      vect_print_slp_graph (MSG_NOTE, vect_location,
+				    SLP_INSTANCE_TREE (new_instance));
 	    }
 
 	  return true;