From: Wei Mi <wmi@google.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Jan Hubicka <hubicka@ucw.cz>, David Li <davidxl@google.com>,
Martin Jambor <mjambor@suse.cz>
Subject: Re: [PATCH, PR61776] verify_flow_info failed: control flow in the middle of basic block with -fprofile-generate
Date: Mon, 28 Jul 2014 06:40:00 -0000 [thread overview]
Message-ID: <CA+4CFy7a=CbTVSnpchKBqQjomw8Fc6kHWK2BkVDBVddyCRwO5A@mail.gmail.com> (raw)
In-Reply-To: <CAFiYyc3TO_o1_taze9F6h+-qNOxmMqCDJ_VZLdDz17MmUfXJzA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]
> 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 for the comment and thanks to Martin's patch. I try the patch.
It works well to address both pr60449 and pr61776 after some
extension. One extension is to replace GF_CALL_LEAF attribute using
GF_CALL_NO_ABNORMAL_GOTO. That is because not only dropping "leaf"
attribute in lto symbol merge could introduce the control flow
verification problem in pr60449, dropping "const/pure" attributes
could introduce the same problem too. It is unnecessary to introduce
per-call attributes for all these three: ECF_LEAF/ECF_CONST/ECF_PURE,
so GF_CALL_NO_ABNORMAL_GOTO is introduced to indicate that a call stmt
has no abnormal goto.
GF_CALL_NO_ABNORMAL_GOTO will be set according to gimple_call_flags()
once gimple call stmt is created, then updated in execute_fixup_cfg
and cleanup_tree_cfg.
I posted the extended patch here. I didn't add the noreturn part in
because it has no direct impact on pr60449 and pr61776. I can help
Martin to test and post that part as an independent patch later.
bootstrap and regression pass on x86_64-linux-gnu. Is it ok?
Thanks,
Wei.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 8988 bytes --]
ChangeLog:
2014-07-27 Martin Jambor <mjambor@suse.cz>
Wei Mi <wmi@google.com>
PR ipa/60449
PR middle-end/61776
* tree-cfgcleanup.c (update_no_abnormal_goto_attr): New function.
(cleanup_tree_cfg_1): Use update_no_abnormal_goto_attr.
* gimple.c (gimple_call_initialize_no_abnormal_goto): New function.
(gimple_build_call_1): Use gimple_call_initialize_no_abnormal_goto.
(gimple_build_call_internal_1): Ditto.
* gimple.h (enum gf_mask): Added GF_NO_ABNORMAL_GOTO.
(gimple_call_set_no_abnormal_goto): New function.
(gimple_call_no_abnormal_goto_p): Ditto.
* tree-cfg.c (call_can_make_abnormal_goto):
Use gimple_call_no_abnormal_goto_p.
(execute_fixup_cfg): Use gimple_call_set_no_abnormal_goto.
2014-07-27 Martin Jambor <mjambor@suse.cz>
Wei Mi <wmi@google.com>
PR ipa/60449
PR middle-end/61776
* testsuite/gcc.dg/pr61776.c: New test.
* testsuite/gcc.dg/lto/pr60449_1.c: New test.
* testsuite/gcc.dg/lto/pr60449_0.c: New test.
Index: tree-cfgcleanup.c
===================================================================
--- tree-cfgcleanup.c (revision 212442)
+++ tree-cfgcleanup.c (working copy)
@@ -621,6 +621,28 @@ split_bbs_on_noreturn_calls (void)
return changed;
}
+/* Update GF_NO_ABNORMAL_GOTO attribute for call stmts in BB according
+ to gimple_call_flags. */
+
+static void
+update_no_abnormal_goto_attr (basic_block bb)
+{
+ gimple_stmt_iterator gsi;
+ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+ {
+ gimple stmt = gsi_stmt (gsi);
+
+ if (!is_gimple_call (stmt))
+ continue;
+
+ int flags = gimple_call_flags (stmt);
+ if ((flags & (ECF_CONST | ECF_PURE)
+ && !(flags & ECF_LOOPING_CONST_OR_PURE))
+ || (flags & ECF_LEAF))
+ gimple_call_set_no_abnormal_goto (stmt, true);
+ }
+}
+
/* Tries to cleanup cfg in basic block BB. Returns true if anything
changes. */
@@ -672,7 +694,10 @@ cleanup_tree_cfg_1 (void)
{
bb = BASIC_BLOCK_FOR_FN (cfun, i);
if (bb)
- retval |= cleanup_tree_cfg_bb (bb);
+ {
+ update_no_abnormal_goto_attr (bb);
+ retval |= cleanup_tree_cfg_bb (bb);
+ }
}
/* Now process the altered blocks, as long as any are available. */
@@ -687,6 +712,7 @@ cleanup_tree_cfg_1 (void)
if (!bb)
continue;
+ update_no_abnormal_goto_attr (bb);
retval |= cleanup_tree_cfg_bb (bb);
/* Rerun split_bbs_on_noreturn_calls, in case we have altered any noreturn
Index: gimple.c
===================================================================
--- gimple.c (revision 212442)
+++ gimple.c (working copy)
@@ -186,6 +186,19 @@ gimple_build_return (tree retval)
return s;
}
+/* Set GF_NO_ABNORMAL_GOTO attribute according to gimple_call_flags(STMT). */
+
+void
+gimple_call_initialize_no_abnormal_goto (gimple stmt)
+{
+ int flags = gimple_call_flags (stmt);
+
+ if ((flags & (ECF_CONST | ECF_PURE)
+ && !(flags & ECF_LOOPING_CONST_OR_PURE))
+ || (flags & ECF_LEAF))
+ gimple_call_set_no_abnormal_goto (stmt, true);
+}
+
/* Reset alias information on call S. */
void
@@ -215,6 +228,7 @@ gimple_build_call_1 (tree fn, unsigned n
gimple_set_op (s, 1, fn);
gimple_call_set_fntype (s, TREE_TYPE (TREE_TYPE (fn)));
gimple_call_reset_alias_info (s);
+ gimple_call_initialize_no_abnormal_goto (s);
return s;
}
@@ -290,6 +304,7 @@ gimple_build_call_internal_1 (enum inter
s->subcode |= GF_CALL_INTERNAL;
gimple_call_set_internal_fn (s, fn);
gimple_call_reset_alias_info (s);
+ gimple_call_initialize_no_abnormal_goto (s);
return s;
}
Index: gimple.h
===================================================================
--- gimple.h (revision 212442)
+++ gimple.h (working copy)
@@ -90,6 +90,7 @@ enum gf_mask {
GF_CALL_NOTHROW = 1 << 4,
GF_CALL_ALLOCA_FOR_VAR = 1 << 5,
GF_CALL_INTERNAL = 1 << 6,
+ GF_CALL_NO_ABNORMAL_GOTO = 1 << 7,
GF_OMP_PARALLEL_COMBINED = 1 << 0,
GF_OMP_FOR_KIND_MASK = (1 << 2) - 1,
GF_OMP_FOR_KIND_FOR = 0,
@@ -2449,7 +2450,28 @@ gimple_call_internal_p (const_gimple gs)
}
-/* Return the target of internal call GS. */
+/* If NO_ABNORMAL_GOTO_P is true mark GIMPLE_CALL S as not having any
+ abnormal goto effect. It doesn't exclude EH. */
+static inline void
+gimple_call_set_no_abnormal_goto (gimple s, bool no_abnormal_goto_p)
+{
+ GIMPLE_CHECK (s, GIMPLE_CALL);
+ if (no_abnormal_goto_p)
+ s->subcode |= GF_CALL_NO_ABNORMAL_GOTO;
+ else
+ s->subcode &= ~GF_CALL_NO_ABNORMAL_GOTO;
+}
+
+/* Return true if call GS calls an func without abnormal goto. Such call
+ could be a stmt in the middle of a bb. */
+
+static inline bool
+gimple_call_no_abnormal_goto_p (const_gimple gs)
+{
+ GIMPLE_CHECK (gs, GIMPLE_CALL);
+ return (gs->subcode & GF_CALL_NO_ABNORMAL_GOTO) != 0;
+}
+
static inline enum internal_fn
gimple_call_internal_fn (const_gimple gs)
Index: tree-cfg.c
===================================================================
--- tree-cfg.c (revision 212442)
+++ tree-cfg.c (working copy)
@@ -2318,15 +2318,11 @@ call_can_make_abnormal_goto (gimple t)
&& !cfun->calls_setjmp)
return false;
- /* Likewise if the call has no side effects. */
- if (!gimple_has_side_effects (t))
+ /* If the call has been marked as having no abnormal goto. */
+ if (gimple_call_no_abnormal_goto_p (t))
return false;
-
- /* Likewise if the called function is leaf. */
- if (gimple_call_flags (t) & ECF_LEAF)
- return false;
-
- return true;
+ else
+ return true;
}
@@ -8485,6 +8481,11 @@ execute_fixup_cfg (void)
}
}
+ if ((flags & (ECF_CONST | ECF_PURE)
+ && !(flags & ECF_LOOPING_CONST_OR_PURE))
+ || (flags & ECF_LEAF))
+ gimple_call_set_no_abnormal_goto (stmt, true);
+
if (flags & ECF_NORETURN
&& fixup_noreturn_call (stmt))
todo |= TODO_cleanup_cfg;
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;
+}
Index: testsuite/gcc.dg/lto/pr60449_1.c
===================================================================
--- testsuite/gcc.dg/lto/pr60449_1.c (revision 0)
+++ testsuite/gcc.dg/lto/pr60449_1.c (revision 0)
@@ -0,0 +1,76 @@
+extern int printf (const char *__restrict __format, ...);
+typedef long int __time_t;
+typedef long int __suseconds_t;
+struct timeval
+ {
+ __time_t tv_sec;
+ __suseconds_t tv_usec;
+ };
+struct timezone
+ {
+ int tz_minuteswest;
+ int tz_dsttime;
+ };
+typedef struct timezone *__restrict __timezone_ptr_t;
+extern int gettimeofday (struct timeval *__restrict __tv,
+ __timezone_ptr_t __tz) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__nonnull__ (1)));
+
+typedef long int __jmp_buf[8];
+typedef struct
+ {
+ unsigned long int __val[(1024 / (8 * sizeof (unsigned long int)))];
+ } __sigset_t;
+struct __jmp_buf_tag
+ {
+ __jmp_buf __jmpbuf;
+ int __mask_was_saved;
+ __sigset_t __saved_mask;
+ };
+typedef struct __jmp_buf_tag jmp_buf[1];
+
+extern int setjmp (jmp_buf __env) __attribute__ ((__nothrow__));
+extern void longjmp (struct __jmp_buf_tag __env[1], int __val)
+ __attribute__ ((__nothrow__)) __attribute__ ((__noreturn__));
+
+extern int bar (void);
+
+int __attribute__ ((noinline, noclone))
+get_input (void)
+{
+ return 0;
+}
+
+static jmp_buf buf;
+
+int foo (void)
+{
+ if (get_input ())
+ longjmp(buf, 1);
+ return 0;
+}
+
+volatile int z;
+
+
+int main (void)
+{
+ struct timeval tv;
+ struct timezone tz;
+
+ bar();
+ if (setjmp (buf))
+ return 1;
+
+ if (!get_input ())
+ {
+ gettimeofday (&tv, &tz);
+ z = 0;
+ printf ("This is from main %i\n", tz.tz_dsttime);
+ }
+
+ foo ();
+ bar ();
+ bar ();
+
+ return 0;
+}
Index: testsuite/gcc.dg/lto/pr60449_0.c
===================================================================
--- testsuite/gcc.dg/lto/pr60449_0.c (revision 0)
+++ testsuite/gcc.dg/lto/pr60449_0.c (revision 0)
@@ -0,0 +1,30 @@
+/* { dg-lto-do link } */
+
+extern int printf (const char *__restrict __format, ...);
+typedef long int __time_t;
+typedef long int __suseconds_t;
+
+struct timeval
+ {
+ __time_t tv_sec;
+ __suseconds_t tv_usec;
+ };
+
+struct timezone
+ {
+ int tz_minuteswest;
+ int tz_dsttime;
+ };
+typedef struct timezone *__restrict __timezone_ptr_t;
+
+extern int gettimeofday (struct timeval *__restrict __tv, __timezone_ptr_t __tz);
+
+int bar (void)
+{
+ struct timeval tv;
+ struct timezone tz;
+
+ gettimeofday (&tv, &tz);
+ printf ("This is from bar %i\n", tz.tz_dsttime);
+ return 5;
+}
next prev parent reply other threads:[~2014-07-28 6:08 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-21 18:21 Wei Mi
2014-07-21 18:21 ` Wei Mi
2014-07-23 9:58 ` Richard Biener
2014-07-23 12:48 ` Martin Jambor
2014-07-23 13:47 ` Richard Biener
2014-07-28 6:40 ` Wei Mi [this message]
2014-08-12 20:56 ` Wei Mi
2014-08-14 14:32 ` Richard Biener
2014-08-19 18:46 ` Wei Mi
2014-08-20 14:18 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+4CFy7a=CbTVSnpchKBqQjomw8Fc6kHWK2BkVDBVddyCRwO5A@mail.gmail.com' \
--to=wmi@google.com \
--cc=davidxl@google.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=mjambor@suse.cz \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).