| Message ID | CA+4CFy7sLjEWstfiHGYAv77iXitaYbRBTRpi+JUwz641VSFhBQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
By the way, the resetting of const/pure flags loop is also executed during profile-useļ¼ but if there is no instrumentation, the reset is unnecessary. The flags are kept until pass_ipa_pure_const fixes them. And because of non-instantaneous ssa update, the fixes are reflected on ssa only after ipa passes finish. If it is agreed that this is a problem, I will address the conservativeness in a separate patch. Regards, Wei.
On Mon, Jul 21, 2014 at 7:06 PM, Wei Mi <wmi@google.com> wrote: > Hi, > > This patch is to fix: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61776 > > It records func decls whose const/pure flags are reset during > instrumentation. After the loop resetting const/pure flags, find out > stmts calling those recorded funcs and perform cfg fixup on them. > > bootstrap and regression test pass on x86_64-linux-gnu. ok for trunk > and gcc-4_9? But fact is that it is _not_ necessary to split the block because there are no outgoing abnormal edges from it. The verifier failure is an artifact from using the same predicates during CFG building and CFG verifying (usually ok, but for this particular case it leads to this issue). So I don't think your patch is the proper way to address this issue (while it certainly works). Instead whether a call can make abnormal gotos should be recorded per-call and stored on the gimple-call. Martin - this is exactly one of the cases your patch would address? Thanks, Richard. > Thanks, > Wei. > > ChangeLog: > > 2014-07-21 Wei Mi <wmi@google.com> > > PR middle-end/61776 > * tree-profile.c (tree_profiling): Fix cfg after the const/pure > flags of some funcs are reset after instrumentation. > > 2014-07-21 Wei Mi <wmi@google.com> > > PR middle-end/61776 > * testsuite/gcc.dg/pr61776.c: New test. > > Index: tree-profile.c > =================================================================== > --- tree-profile.c (revision 212442) > +++ tree-profile.c (working copy) > @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. > #include "target.h" > #include "tree-cfgcleanup.h" > #include "tree-nested.h" > +#include "pointer-set.h" > > static GTY(()) tree gcov_type_node; > static GTY(()) tree tree_interval_profiler_fn; > @@ -562,6 +563,9 @@ static unsigned int > tree_profiling (void) > { > struct cgraph_node *node; > + int i; > + struct pointer_set_t *modified_constpure_decls; > + vec<gimple> modified_constpure_stmts; > > /* This is a small-ipa pass that gets called only once, from > cgraphunit.c:ipa_passes(). */ > @@ -603,6 +607,9 @@ tree_profiling (void) > pop_cfun (); > } > > + modified_constpure_decls = pointer_set_create (); > + modified_constpure_stmts.create (0); > + > /* Drop pure/const flags from instrumented functions. */ > FOR_EACH_DEFINED_FUNCTION (node) > { > @@ -615,6 +622,11 @@ tree_profiling (void) > if (DECL_SOURCE_LOCATION (node->decl) == BUILTINS_LOCATION) > continue; > > + /* If the const/pure flag of node is about to change, record > + node->decl in modified_constpure_decls. */ > + if (DECL_PURE_P (node->decl) || TREE_READONLY (node->decl)) > + pointer_set_insert (modified_constpure_decls, node->decl); > + > cgraph_set_const_flag (node, false, false); > cgraph_set_pure_flag (node, false, false); > } > @@ -623,6 +635,7 @@ tree_profiling (void) > FOR_EACH_DEFINED_FUNCTION (node) > { > basic_block bb; > + gimple stmt; > > if (!gimple_has_body_p (node->decl) > || !(!node->clone_of > @@ -642,10 +655,29 @@ tree_profiling (void) > { > gimple stmt = gsi_stmt (gsi); > if (is_gimple_call (stmt)) > - update_stmt (stmt); > + { > + tree decl = gimple_call_fndecl(stmt); > + if (decl && pointer_set_contains (modified_constpure_decls, > + decl)) > + modified_constpure_stmts.safe_push (stmt); > + update_stmt (stmt); > + } > } > } > > + /* The const/pure flag of the decl of call stmt in > modified_constpure_stmts > + is changed because of instrumentation. Split block if the > call stmt is not > + the last stmt of bb and the call stmt ends bb. */ > + FOR_EACH_VEC_ELT (modified_constpure_stmts, i, stmt) > + { > + basic_block bb = gimple_bb (stmt); > + > + if (stmt != gsi_stmt (gsi_last_bb (bb)) > + && stmt_ends_bb_p (stmt)) > + split_block (bb, stmt); > + } > + modified_constpure_stmts.release (); > + > /* re-merge split blocks. */ > cleanup_tree_cfg (); > update_ssa (TODO_update_ssa); > @@ -657,6 +689,7 @@ tree_profiling (void) > > handle_missing_profiles (); > > + pointer_set_destroy (modified_constpure_decls); > del_node_map (); > return 0; > } > Index: testsuite/gcc.dg/pr61776.c > =================================================================== > --- testsuite/gcc.dg/pr61776.c (revision 0) > +++ testsuite/gcc.dg/pr61776.c (revision 0) > @@ -0,0 +1,27 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fprofile-generate" } */ > + > +#include <setjmp.h> > + > +int cond1, cond2; > + > +int goo() __attribute__((noinline)); > + > +int goo() { > + if (cond1) > + return 1; > + else > + return 2; > +} > + > +jmp_buf env; > +int foo() { > + int a; > + > + setjmp(env); > + if (cond2) > + a = goo(); > + else > + a = 3; > + return a; > +}
Hi, On Wed, Jul 23, 2014 at 11:51:37AM +0200, Richard Biener wrote: > On Mon, Jul 21, 2014 at 7:06 PM, Wei Mi <wmi@google.com> wrote: > > Hi, > > > > This patch is to fix: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61776 > > > > It records func decls whose const/pure flags are reset during > > instrumentation. After the loop resetting const/pure flags, find out > > stmts calling those recorded funcs and perform cfg fixup on them. > > > > bootstrap and regression test pass on x86_64-linux-gnu. ok for trunk > > and gcc-4_9? > > But fact is that it is _not_ necessary to split the block because there > are no outgoing abnormal edges from it. > > The verifier failure is an artifact from using the same predicates during > CFG building and CFG verifying (usually ok, but for this particular > case it leads to this issue). > > So I don't think your patch is the proper way to address this issue > (while it certainly works). > > Instead whether a call can make abnormal gotos should be recorded > per-call and stored on the gimple-call. Martin - this is exactly > one of the cases your patch would address? No, my patch, which is attached to PR 60449 in bugzilla, introduces leaf and noreturn gimple statement attributes and uses them to deal with what really seems to be a very similar problem. However, my patch does not really look like 4.9 material. Thanks, Martin
On Wed, Jul 23, 2014 at 2:11 PM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Wed, Jul 23, 2014 at 11:51:37AM +0200, Richard Biener wrote: >> On Mon, Jul 21, 2014 at 7:06 PM, Wei Mi <wmi@google.com> wrote: >> > Hi, >> > >> > This patch is to fix: >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61776 >> > >> > It records func decls whose const/pure flags are reset during >> > instrumentation. After the loop resetting const/pure flags, find out >> > stmts calling those recorded funcs and perform cfg fixup on them. >> > >> > bootstrap and regression test pass on x86_64-linux-gnu. ok for trunk >> > and gcc-4_9? >> >> But fact is that it is _not_ necessary to split the block because there >> are no outgoing abnormal edges from it. >> >> The verifier failure is an artifact from using the same predicates during >> CFG building and CFG verifying (usually ok, but for this particular >> case it leads to this issue). >> >> So I don't think your patch is the proper way to address this issue >> (while it certainly works). >> >> Instead whether a call can make abnormal gotos should be recorded >> per-call and stored on the gimple-call. Martin - this is exactly >> one of the cases your patch would address? > > No, my patch, which is attached to PR 60449 in bugzilla, introduces > leaf and noreturn gimple statement attributes and uses them to deal > with what really seems to be a very similar problem. However, my > patch does not really look like 4.9 material. It doesn't really matter for 4.9 if you built that with release checking. Richard. > Thanks, > > Martin
Index: tree-profile.c =================================================================== --- tree-profile.c (revision 212442) +++ tree-profile.c (working copy) @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. #include "target.h" #include "tree-cfgcleanup.h" #include "tree-nested.h" +#include "pointer-set.h" static GTY(()) tree gcov_type_node; static GTY(()) tree tree_interval_profiler_fn; @@ -562,6 +563,9 @@ static unsigned int tree_profiling (void) { struct cgraph_node *node; + int i; + struct pointer_set_t *modified_constpure_decls; + vec<gimple> modified_constpure_stmts; /* This is a small-ipa pass that gets called only once, from cgraphunit.c:ipa_passes(). */ @@ -603,6 +607,9 @@ tree_profiling (void) pop_cfun (); } + modified_constpure_decls = pointer_set_create (); + modified_constpure_stmts.create (0); + /* Drop pure/const flags from instrumented functions. */ FOR_EACH_DEFINED_FUNCTION (node) { @@ -615,6 +622,11 @@ tree_profiling (void) if (DECL_SOURCE_LOCATION (node->decl) == BUILTINS_LOCATION) continue; + /* If the const/pure flag of node is about to change, record + node->decl in modified_constpure_decls. */ + if (DECL_PURE_P (node->decl) || TREE_READONLY (node->decl)) + pointer_set_insert (modified_constpure_decls, node->decl); + cgraph_set_const_flag (node, false, false); cgraph_set_pure_flag (node, false, false); } @@ -623,6 +635,7 @@ tree_profiling (void) FOR_EACH_DEFINED_FUNCTION (node) { basic_block bb; + gimple stmt; if (!gimple_has_body_p (node->decl) || !(!node->clone_of @@ -642,10 +655,29 @@ tree_profiling (void) { gimple stmt = gsi_stmt (gsi); if (is_gimple_call (stmt)) - update_stmt (stmt); + { + tree decl = gimple_call_fndecl(stmt); + if (decl && pointer_set_contains (modified_constpure_decls, + decl)) + modified_constpure_stmts.safe_push (stmt); + update_stmt (stmt); + } } } + /* The const/pure flag of the decl of call stmt in modified_constpure_stmts + is changed because of instrumentation. Split block if the call stmt is not + the last stmt of bb and the call stmt ends bb. */ + FOR_EACH_VEC_ELT (modified_constpure_stmts, i, stmt) + { + basic_block bb = gimple_bb (stmt); + + if (stmt != gsi_stmt (gsi_last_bb (bb)) + && stmt_ends_bb_p (stmt)) + split_block (bb, stmt); + } + modified_constpure_stmts.release (); + /* re-merge split blocks. */ cleanup_tree_cfg (); update_ssa (TODO_update_ssa); @@ -657,6 +689,7 @@ tree_profiling (void) handle_missing_profiles (); + pointer_set_destroy (modified_constpure_decls); del_node_map (); return 0; } Index: testsuite/gcc.dg/pr61776.c =================================================================== --- testsuite/gcc.dg/pr61776.c (revision 0) +++ testsuite/gcc.dg/pr61776.c (revision 0) @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fprofile-generate" } */ + +#include <setjmp.h> + +int cond1, cond2; + +int goo() __attribute__((noinline)); + +int goo() { + if (cond1) + return 1; + else + return 2; +} + +jmp_buf env; +int foo() { + int a; + + setjmp(env); + if (cond2) + a = goo(); + else + a = 3; + return a; +}
Hi, This patch is to fix: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61776 It records func decls whose const/pure flags are reset during instrumentation. After the loop resetting const/pure flags, find out stmts calling those recorded funcs and perform cfg fixup on them. bootstrap and regression test pass on x86_64-linux-gnu. ok for trunk and gcc-4_9? Thanks, Wei. ChangeLog: 2014-07-21 Wei Mi <wmi@google.com> PR middle-end/61776 * tree-profile.c (tree_profiling): Fix cfg after the const/pure flags of some funcs are reset after instrumentation. 2014-07-21 Wei Mi <wmi@google.com> PR middle-end/61776 * testsuite/gcc.dg/pr61776.c: New test.