| Message ID | Yj3ZChOQo29GJpj/@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [13] Enable match.pd dumping with -fdump-tree-original | expand |
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;
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.
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;
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.