Patchwork [PR61776] verify_flow_info failed: control flow in the middle of basic block with -fprofile-generate

login
register
mail settings
Submitter Wei Mi
Date July 21, 2014, 5:06 p.m.
Message ID <CA+4CFy7sLjEWstfiHGYAv77iXitaYbRBTRpi+JUwz641VSFhBQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/372168/
State New
Headers show

Comments

Wei Mi - July 21, 2014, 5:06 p.m.
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.
Wei Mi - July 21, 2014, 6:20 p.m.
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.
Richard Guenther - July 23, 2014, 9:51 a.m.
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;
> +}
Martin Jambor - July 23, 2014, 12:11 p.m.
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
Richard Guenther - July 23, 2014, 1:47 p.m.
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

Patch

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;
+}