[PATCH][GCC 13] Enable match.pd dumping with -fdump-tree-original

[PATCH][GCC 13] Enable match.pd dumping with -fdump-tree-original

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 3295 bytes --]

Hi,

I noticed that, while the C/C++ frontends invoke the GENERIC match.pd
simplifications to do early folding, the debug output from
generic-match.cc does not appear in the -fdump-tree-original output,
even with -fdump-tree-original-folding or -fdump-tree-original-all. This
patch fixes that.

For example, before the patch, for the following code:

int a[2];
void bar ();
void f()
{
    if ((unsigned long)(a + 1) == 0)
        bar ();
}

on AArch64 at -O0, -fdump-tree-original-all would give:

;; Function f (null)
;; enabled by -tree-original


{
  if (0)
    {
      bar ();
    }
}

After the patch, we get:

Applying pattern match.pd:3774, generic-match.cc:24535
Matching expression match.pd:146, generic-match.cc:23
Applying pattern match.pd:5638, generic-match.cc:13388

;; Function f (null)
;; enabled by -tree-original


{
  if (0)
    {
      bar ();
    }
}

The reason we don't get the match.pd output as it stands, is that the
original dump is treated specially in c-opts.cc: it gets its own state
which is independent from that used by other dump files in the compiler.
Like most of the compiler, the generated generic-match.cc has code of
the form:

  if (dump_file && (dump_flags & TDF_FOLDING))
    fprintf (dump_file, ...);

But, as it stands, -fdump-tree-original has its own FILE * and flags in
c-opts.cc (original_dump_{file,flags}) and never touches the global
dump_{file,flags} (managed by dumpfile.{h,cc}). This patch adjusts the
code in c-opts.cc to use the main dump infrastructure used by the rest
of the compiler, instead of treating the original dump specially.

We take the opportunity to make a small refactor: the code in
c-gimplify.cc:c_genericize can, with this change, use the global dump
infrastructure to get the original dump file and flags instead of using
the bespoke get_dump_info function implemented in c-opts.cc. With this
change, we remove the only use of get_dump_info, so this can be removed.

Note that we also fix a leak of the original dump file in
c_common_parse_file. I originally thought it might be possible to
achieve this with only one static call to dump_finish () (by simply
moving it earlier in the loop), but unfortunately the dump file is
required to be open while c_parse_final_cleanups runs, as we (e.g.)
perform some template instantiations here for C++, which need to appear
in the original dump file.

We adjust cgraph_node::get_create to avoid introducing noise in the
original dump file: without this, these "Introduced new external node"
lines start appearing in the original dump files, which breaks tests
that do a scan-tree-dump-times on the original dump looking for a
certain function name.

Bootstrapped/regtested on aarch64-linux-gnu, OK for GCC 13?

Thanks,
Alex

gcc/c-family/ChangeLog:

	* c-common.h (get_dump_info): Delete.
	* c-gimplify.cc (c_genericize): Get TDI_original dump file info
	from the global dump_manager instead of the (now obsolete)
	get_dump_info.
	* c-opts.cc (original_dump_file): Delete.
	(original_dump_flags): Delete.
	(c_common_parse_file): Switch to using global dump_manager to
	manage the original dump file; fix leak of dump file.
	(get_dump_info): Delete.

gcc/ChangeLog:

	* cgraph.cc (cgraph_node::get_create): Don't dump if the current
	dump file is that of -fdump-tree-original.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 4953 bytes --]

diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 52a85bfb783..b829cdbfe28 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -950,7 +950,6 @@ extern bool c_common_post_options (const char **);
 extern bool c_common_init (void);
 extern void c_common_finish (void);
 extern void c_common_parse_file (void);
-extern FILE *get_dump_info (int, dump_flags_t *);
 extern alias_set_type c_common_get_alias_set (tree);
 extern void c_register_builtin_type (tree, const char*);
 extern bool c_promoting_integer_type_p (const_tree);
diff --git a/gcc/c-family/c-gimplify.cc b/gcc/c-family/c-gimplify.cc
index a00b0a02dcc..a6f26c9b0d3 100644
--- a/gcc/c-family/c-gimplify.cc
+++ b/gcc/c-family/c-gimplify.cc
@@ -40,6 +40,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "dumpfile.h"
 #include "c-ubsan.h"
 #include "tree-nested.h"
+#include "context.h"
 
 /*  The gimplification pass converts the language-dependent trees
     (ld-trees) emitted by the parser into language-independent trees
@@ -552,6 +553,7 @@ c_genericize_control_r (tree *stmt_p, int *walk_subtrees, void *data)
 void
 c_genericize (tree fndecl)
 {
+  dump_file_info *dfi;
   FILE *dump_orig;
   dump_flags_t local_dump_flags;
   struct cgraph_node *cgn;
@@ -581,7 +583,9 @@ c_genericize (tree fndecl)
 				  do_warn_duplicated_branches_r, NULL);
 
   /* Dump the C-specific tree IR.  */
-  dump_orig = get_dump_info (TDI_original, &local_dump_flags);
+  dfi = g->get_dumps ()->get_dump_file_info (TDI_original);
+  dump_orig = dfi->pstream;
+  local_dump_flags = dfi->pflags;
   if (dump_orig)
     {
       fprintf (dump_orig, "\n;; Function %s",
diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
index a341a061758..09a16b20f36 100644
--- a/gcc/c-family/c-opts.cc
+++ b/gcc/c-family/c-opts.cc
@@ -41,6 +41,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "mkdeps.h"
 #include "dumpfile.h"
 #include "file-prefix-map.h"    /* add_*_prefix_map()  */
+#include "context.h"
 
 #ifndef DOLLARS_IN_IDENTIFIERS
 # define DOLLARS_IN_IDENTIFIERS true
@@ -100,10 +101,6 @@ static size_t deferred_count;
 /* Number of deferred options scanned for -include.  */
 static size_t include_cursor;
 
-/* Dump files/flags to use during parsing.  */
-static FILE *original_dump_file = NULL;
-static dump_flags_t original_dump_flags;
-
 /* Whether any standard preincluded header has been preincluded.  */
 static bool done_preinclude;
 
@@ -1226,15 +1223,13 @@ c_common_init (void)
 void
 c_common_parse_file (void)
 {
-  unsigned int i;
-
-  i = 0;
-  for (;;)
+  auto dumps = g->get_dumps ();
+  for (unsigned int i = 0;;)
     {
       c_finish_options ();
       /* Open the dump file to use for the original dump output
          here, to be used during parsing for the current file.  */
-      original_dump_file = dump_begin (TDI_original, &original_dump_flags);
+      dumps->dump_start (TDI_original, &dump_flags);
       pch_init ();
       push_file_scope ();
       c_parse_file ();
@@ -1248,29 +1243,15 @@ c_common_parse_file (void)
       cpp_clear_file_cache (parse_in);
       this_input_filename
 	= cpp_read_main_file (parse_in, in_fnames[i]);
-      if (original_dump_file)
-        {
-          dump_end (TDI_original, original_dump_file);
-          original_dump_file = NULL;
-        }
       /* If an input file is missing, abandon further compilation.
 	 cpplib has issued a diagnostic.  */
       if (!this_input_filename)
 	break;
+      dumps->dump_finish (TDI_original);
     }
 
   c_parse_final_cleanups ();
-}
-
-/* Returns the appropriate dump file for PHASE to dump with FLAGS.  */
-
-FILE *
-get_dump_info (int phase, dump_flags_t *flags)
-{
-  gcc_assert (phase == TDI_original);
-
-  *flags = original_dump_flags;
-  return original_dump_file;
+  dumps->dump_finish (TDI_original);
 }
 
 /* Common finish hook for the C, ObjC and C++ front ends.  */
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index b923a59ab0c..10786b1e372 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -537,6 +537,11 @@ cgraph_node::get_create (tree decl)
     return first_clone;
 
   cgraph_node *node = cgraph_node::create (decl);
+
+  /* Avoid noise in -fdump-tree-original dump file.  */
+  const bool should_dump = dump_file
+    && !g->get_dumps ()->get_dump_file_info (TDI_original)->pstate;
+
   if (first_clone)
     {
       first_clone->clone_of = node;
@@ -544,12 +549,12 @@ cgraph_node::get_create (tree decl)
       node->order = first_clone->order;
       symtab->symtab_prevail_in_asm_name_hash (node);
       node->decl->decl_with_vis.symtab_node = node;
-      if (dump_file)
+      if (should_dump)
 	fprintf (dump_file, "Introduced new external node "
 		 "(%s) and turned into root of the clone tree.\n",
 		 node->dump_name ());
     }
-  else if (dump_file)
+  else if (should_dump)
     fprintf (dump_file, "Introduced new external node "
 	     "(%s).\n", node->dump_name ());
   return node;

Re: [PATCH][GCC 13] Enable match.pd dumping with -fdump-tree-original

[Alex Coplan]

Ping.

https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592307.html

On 25/03/2022 15:07, Alex Coplan via Gcc-patches wrote:
> Hi,
> 
> I noticed that, while the C/C++ frontends invoke the GENERIC match.pd
> simplifications to do early folding, the debug output from
> generic-match.cc does not appear in the -fdump-tree-original output,
> even with -fdump-tree-original-folding or -fdump-tree-original-all. This
> patch fixes that.
> 
> For example, before the patch, for the following code:
> 
> int a[2];
> void bar ();
> void f()
> {
>     if ((unsigned long)(a + 1) == 0)
>         bar ();
> }
> 
> on AArch64 at -O0, -fdump-tree-original-all would give:
> 
> ;; Function f (null)
> ;; enabled by -tree-original
> 
> 
> {
>   if (0)
>     {
>       bar ();
>     }
> }
> 
> After the patch, we get:
> 
> Applying pattern match.pd:3774, generic-match.cc:24535
> Matching expression match.pd:146, generic-match.cc:23
> Applying pattern match.pd:5638, generic-match.cc:13388
> 
> ;; Function f (null)
> ;; enabled by -tree-original
> 
> 
> {
>   if (0)
>     {
>       bar ();
>     }
> }
> 
> The reason we don't get the match.pd output as it stands, is that the
> original dump is treated specially in c-opts.cc: it gets its own state
> which is independent from that used by other dump files in the compiler.
> Like most of the compiler, the generated generic-match.cc has code of
> the form:
> 
>   if (dump_file && (dump_flags & TDF_FOLDING))
>     fprintf (dump_file, ...);
> 
> But, as it stands, -fdump-tree-original has its own FILE * and flags in
> c-opts.cc (original_dump_{file,flags}) and never touches the global
> dump_{file,flags} (managed by dumpfile.{h,cc}). This patch adjusts the
> code in c-opts.cc to use the main dump infrastructure used by the rest
> of the compiler, instead of treating the original dump specially.
> 
> We take the opportunity to make a small refactor: the code in
> c-gimplify.cc:c_genericize can, with this change, use the global dump
> infrastructure to get the original dump file and flags instead of using
> the bespoke get_dump_info function implemented in c-opts.cc. With this
> change, we remove the only use of get_dump_info, so this can be removed.
> 
> Note that we also fix a leak of the original dump file in
> c_common_parse_file. I originally thought it might be possible to
> achieve this with only one static call to dump_finish () (by simply
> moving it earlier in the loop), but unfortunately the dump file is
> required to be open while c_parse_final_cleanups runs, as we (e.g.)
> perform some template instantiations here for C++, which need to appear
> in the original dump file.
> 
> We adjust cgraph_node::get_create to avoid introducing noise in the
> original dump file: without this, these "Introduced new external node"
> lines start appearing in the original dump files, which breaks tests
> that do a scan-tree-dump-times on the original dump looking for a
> certain function name.
> 
> Bootstrapped/regtested on aarch64-linux-gnu, OK for GCC 13?
> 
> Thanks,
> Alex
> 
> gcc/c-family/ChangeLog:
> 
> 	* c-common.h (get_dump_info): Delete.
> 	* c-gimplify.cc (c_genericize): Get TDI_original dump file info
> 	from the global dump_manager instead of the (now obsolete)
> 	get_dump_info.
> 	* c-opts.cc (original_dump_file): Delete.
> 	(original_dump_flags): Delete.
> 	(c_common_parse_file): Switch to using global dump_manager to
> 	manage the original dump file; fix leak of dump file.
> 	(get_dump_info): Delete.
> 
> gcc/ChangeLog:
> 
> 	* cgraph.cc (cgraph_node::get_create): Don't dump if the current
> 	dump file is that of -fdump-tree-original.

> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> index 52a85bfb783..b829cdbfe28 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -950,7 +950,6 @@ extern bool c_common_post_options (const char **);
>  extern bool c_common_init (void);
>  extern void c_common_finish (void);
>  extern void c_common_parse_file (void);
> -extern FILE *get_dump_info (int, dump_flags_t *);
>  extern alias_set_type c_common_get_alias_set (tree);
>  extern void c_register_builtin_type (tree, const char*);
>  extern bool c_promoting_integer_type_p (const_tree);
> diff --git a/gcc/c-family/c-gimplify.cc b/gcc/c-family/c-gimplify.cc
> index a00b0a02dcc..a6f26c9b0d3 100644
> --- a/gcc/c-family/c-gimplify.cc
> +++ b/gcc/c-family/c-gimplify.cc
> @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "dumpfile.h"
>  #include "c-ubsan.h"
>  #include "tree-nested.h"
> +#include "context.h"
>  
>  /*  The gimplification pass converts the language-dependent trees
>      (ld-trees) emitted by the parser into language-independent trees
> @@ -552,6 +553,7 @@ c_genericize_control_r (tree *stmt_p, int *walk_subtrees, void *data)
>  void
>  c_genericize (tree fndecl)
>  {
> +  dump_file_info *dfi;
>    FILE *dump_orig;
>    dump_flags_t local_dump_flags;
>    struct cgraph_node *cgn;
> @@ -581,7 +583,9 @@ c_genericize (tree fndecl)
>  				  do_warn_duplicated_branches_r, NULL);
>  
>    /* Dump the C-specific tree IR.  */
> -  dump_orig = get_dump_info (TDI_original, &local_dump_flags);
> +  dfi = g->get_dumps ()->get_dump_file_info (TDI_original);
> +  dump_orig = dfi->pstream;
> +  local_dump_flags = dfi->pflags;
>    if (dump_orig)
>      {
>        fprintf (dump_orig, "\n;; Function %s",
> diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
> index a341a061758..09a16b20f36 100644
> --- a/gcc/c-family/c-opts.cc
> +++ b/gcc/c-family/c-opts.cc
> @@ -41,6 +41,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "mkdeps.h"
>  #include "dumpfile.h"
>  #include "file-prefix-map.h"    /* add_*_prefix_map()  */
> +#include "context.h"
>  
>  #ifndef DOLLARS_IN_IDENTIFIERS
>  # define DOLLARS_IN_IDENTIFIERS true
> @@ -100,10 +101,6 @@ static size_t deferred_count;
>  /* Number of deferred options scanned for -include.  */
>  static size_t include_cursor;
>  
> -/* Dump files/flags to use during parsing.  */
> -static FILE *original_dump_file = NULL;
> -static dump_flags_t original_dump_flags;
> -
>  /* Whether any standard preincluded header has been preincluded.  */
>  static bool done_preinclude;
>  
> @@ -1226,15 +1223,13 @@ c_common_init (void)
>  void
>  c_common_parse_file (void)
>  {
> -  unsigned int i;
> -
> -  i = 0;
> -  for (;;)
> +  auto dumps = g->get_dumps ();
> +  for (unsigned int i = 0;;)
>      {
>        c_finish_options ();
>        /* Open the dump file to use for the original dump output
>           here, to be used during parsing for the current file.  */
> -      original_dump_file = dump_begin (TDI_original, &original_dump_flags);
> +      dumps->dump_start (TDI_original, &dump_flags);
>        pch_init ();
>        push_file_scope ();
>        c_parse_file ();
> @@ -1248,29 +1243,15 @@ c_common_parse_file (void)
>        cpp_clear_file_cache (parse_in);
>        this_input_filename
>  	= cpp_read_main_file (parse_in, in_fnames[i]);
> -      if (original_dump_file)
> -        {
> -          dump_end (TDI_original, original_dump_file);
> -          original_dump_file = NULL;
> -        }
>        /* If an input file is missing, abandon further compilation.
>  	 cpplib has issued a diagnostic.  */
>        if (!this_input_filename)
>  	break;
> +      dumps->dump_finish (TDI_original);
>      }
>  
>    c_parse_final_cleanups ();
> -}
> -
> -/* Returns the appropriate dump file for PHASE to dump with FLAGS.  */
> -
> -FILE *
> -get_dump_info (int phase, dump_flags_t *flags)
> -{
> -  gcc_assert (phase == TDI_original);
> -
> -  *flags = original_dump_flags;
> -  return original_dump_file;
> +  dumps->dump_finish (TDI_original);
>  }
>  
>  /* Common finish hook for the C, ObjC and C++ front ends.  */
> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
> index b923a59ab0c..10786b1e372 100644
> --- a/gcc/cgraph.cc
> +++ b/gcc/cgraph.cc
> @@ -537,6 +537,11 @@ cgraph_node::get_create (tree decl)
>      return first_clone;
>  
>    cgraph_node *node = cgraph_node::create (decl);
> +
> +  /* Avoid noise in -fdump-tree-original dump file.  */
> +  const bool should_dump = dump_file
> +    && !g->get_dumps ()->get_dump_file_info (TDI_original)->pstate;
> +
>    if (first_clone)
>      {
>        first_clone->clone_of = node;
> @@ -544,12 +549,12 @@ cgraph_node::get_create (tree decl)
>        node->order = first_clone->order;
>        symtab->symtab_prevail_in_asm_name_hash (node);
>        node->decl->decl_with_vis.symtab_node = node;
> -      if (dump_file)
> +      if (should_dump)
>  	fprintf (dump_file, "Introduced new external node "
>  		 "(%s) and turned into root of the clone tree.\n",
>  		 node->dump_name ());
>      }
> -  else if (dump_file)
> +  else if (should_dump)
>      fprintf (dump_file, "Introduced new external node "
>  	     "(%s).\n", node->dump_name ());
>    return node;

Re: [PATCH][GCC 13] Enable match.pd dumping with -fdump-tree-original

[Richard Biener]

On Fri, Mar 25, 2022 at 4:08 PM Alex Coplan via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> I noticed that, while the C/C++ frontends invoke the GENERIC match.pd
> simplifications to do early folding, the debug output from
> generic-match.cc does not appear in the -fdump-tree-original output,
> even with -fdump-tree-original-folding or -fdump-tree-original-all. This
> patch fixes that.
>
> For example, before the patch, for the following code:
>
> int a[2];
> void bar ();
> void f()
> {
>     if ((unsigned long)(a + 1) == 0)
>         bar ();
> }
>
> on AArch64 at -O0, -fdump-tree-original-all would give:
>
> ;; Function f (null)
> ;; enabled by -tree-original
>
>
> {
>   if (0)
>     {
>       bar ();
>     }
> }
>
> After the patch, we get:
>
> Applying pattern match.pd:3774, generic-match.cc:24535
> Matching expression match.pd:146, generic-match.cc:23
> Applying pattern match.pd:5638, generic-match.cc:13388
>
> ;; Function f (null)
> ;; enabled by -tree-original
>
>
> {
>   if (0)
>     {
>       bar ();
>     }
> }
>
> The reason we don't get the match.pd output as it stands, is that the
> original dump is treated specially in c-opts.cc: it gets its own state
> which is independent from that used by other dump files in the compiler.
> Like most of the compiler, the generated generic-match.cc has code of
> the form:
>
>   if (dump_file && (dump_flags & TDF_FOLDING))
>     fprintf (dump_file, ...);
>
> But, as it stands, -fdump-tree-original has its own FILE * and flags in
> c-opts.cc (original_dump_{file,flags}) and never touches the global
> dump_{file,flags} (managed by dumpfile.{h,cc}). This patch adjusts the
> code in c-opts.cc to use the main dump infrastructure used by the rest
> of the compiler, instead of treating the original dump specially.
>
> We take the opportunity to make a small refactor: the code in
> c-gimplify.cc:c_genericize can, with this change, use the global dump
> infrastructure to get the original dump file and flags instead of using
> the bespoke get_dump_info function implemented in c-opts.cc. With this
> change, we remove the only use of get_dump_info, so this can be removed.
>
> Note that we also fix a leak of the original dump file in
> c_common_parse_file. I originally thought it might be possible to
> achieve this with only one static call to dump_finish () (by simply
> moving it earlier in the loop), but unfortunately the dump file is
> required to be open while c_parse_final_cleanups runs, as we (e.g.)
> perform some template instantiations here for C++, which need to appear
> in the original dump file.
>
> We adjust cgraph_node::get_create to avoid introducing noise in the
> original dump file: without this, these "Introduced new external node"
> lines start appearing in the original dump files, which breaks tests
> that do a scan-tree-dump-times on the original dump looking for a
> certain function name.
>
> Bootstrapped/regtested on aarch64-linux-gnu, OK for GCC 13?

Thanks for tackling this - the only part that I don't like is the cgraph.cc
one.   Can't we instead gate the dumping on symtab->state != PARSING
or symtab->state > CONSTRUCTION?

Richard.

> Thanks,
> Alex
>
> gcc/c-family/ChangeLog:
>
>         * c-common.h (get_dump_info): Delete.
>         * c-gimplify.cc (c_genericize): Get TDI_original dump file info
>         from the global dump_manager instead of the (now obsolete)
>         get_dump_info.
>         * c-opts.cc (original_dump_file): Delete.
>         (original_dump_flags): Delete.
>         (c_common_parse_file): Switch to using global dump_manager to
>         manage the original dump file; fix leak of dump file.
>         (get_dump_info): Delete.
>
> gcc/ChangeLog:
>
>         * cgraph.cc (cgraph_node::get_create): Don't dump if the current
>         dump file is that of -fdump-tree-original.