[PATCH] Add debug (slp_tree) and dump infrastructure for this

[David Malcolm]

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