* [PATCH, PR81192] Fix sigsegv in find_same_succ_bb
@ 2017-07-02 22:26 Tom de Vries
2017-07-02 22:49 ` [PATCH, PR81192] Don't tail-merge blocks from different loops Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2017-07-02 22:26 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]
[ Trying again with before.svg instead of before.pdf ]
Hi,
consider this test-case:
...
unsigned a;
int b, c;
static int
fn1 (int p1, int p2)
{
return p1 > 2147483647 - p2 ? p1 : p1 + p2;
}
void
fn2 (void)
{
int j;
a = 30;
for (; a;)
for (; c; b = fn1 (j, 1))
;
}
...
When compiling the test-case with -Os, just before tail-merge it looks
as in before.svg.
During tail-merge, it runs into a sigsegv.
What happens is the following:
- tail-merge decides to merge blocks 4 and 6, and removes block 6.
- bb8, a predecessor of block 6, is marked as member of
deleted_bb_preds.
- during update_worklist, same_succ_flush_bb is called for bb8
- same_succ_flush_bb runs into a sigsegv because
BB_SAME_SUCC (bb8) == NULL
- the reason that BB_SAME_SUCC (bb8) == NULL, is because it hit the
bb->loop_father->latch == bb clause in find_same_succ_bb at the start
of the tail-merge pass.
This patch fixes the sigsegv by doing an early-out in same_succ_flush_bb
if BB_SAME_SUCC () == NULL.
Bootstrapped and reg-tested on x86_64.
OK for trunk and gcc-[567]-branch?
Thanks,
- Tom
[-- Attachment #2: before.svg --]
[-- Type: image/svg+xml, Size: 14186 bytes --]
[-- Attachment #3: 0001-Fix-sigsegv-in-find_same_succ_bb.patch --]
[-- Type: text/x-patch, Size: 1373 bytes --]
Fix sigsegv in find_same_succ_bb
2017-06-30 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/81192
* tree-ssa-tail-merge.c (same_succ_flush_bb): Handle
BB_SAME_SUCC (bb) == NULL.
* gcc.dg/pr81192.c: New test.
---
gcc/testsuite/gcc.dg/pr81192.c | 22 ++++++++++++++++++++++
gcc/tree-ssa-tail-merge.c | 3 +++
2 files changed, 25 insertions(+)
diff --git a/gcc/testsuite/gcc.dg/pr81192.c b/gcc/testsuite/gcc.dg/pr81192.c
new file mode 100644
index 0000000..57eb478
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr81192.c
@@ -0,0 +1,22 @@
+/* { dg-options "-Os -fdump-tree-pre-details" } */
+
+unsigned a;
+int b, c;
+
+static int
+fn1 (int p1, int p2)
+{
+ return p1 > 2147483647 - p2 ? p1 : p1 + p2;
+}
+
+void
+fn2 (void)
+{
+ int j;
+ a = 30;
+ for (; a;)
+ for (; c; b = fn1 (j, 1))
+ ;
+}
+
+/* { dg-final { scan-tree-dump-times "(?n)find_duplicates: <bb .*> duplicate of <bb .*>" 1 "pre" } } */
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index f6c9878..bb8a308 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -809,6 +809,9 @@ static void
same_succ_flush_bb (basic_block bb)
{
same_succ *same = BB_SAME_SUCC (bb);
+ if (! same)
+ return;
+
BB_SAME_SUCC (bb) = NULL;
if (bitmap_single_bit_set_p (same->bbs))
same_succ_htab->remove_elt_with_hash (same, same->hashval);
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH, PR81192] Don't tail-merge blocks from different loops
2017-07-02 22:26 [PATCH, PR81192] Fix sigsegv in find_same_succ_bb Tom de Vries
@ 2017-07-02 22:49 ` Tom de Vries
2017-07-02 23:12 ` [PATCH, PR69468] Ignore EDGE_{DFS_BACK,EXECUTABLE} in tail-merge Tom de Vries
2017-07-03 7:01 ` [PATCH, PR81192] Don't tail-merge blocks from different loops Richard Biener
0 siblings, 2 replies; 6+ messages in thread
From: Tom de Vries @ 2017-07-02 22:49 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]
[ was: Re: [PATCH, PR81192] Fix sigsegv in find_same_succ_bb ]
On 07/03/2017 12:26 AM, Tom de Vries wrote:
> [ Trying again with before.svg instead of before.pdf ]
>
> Hi,
>
> consider this test-case:
> ...
> unsigned a;
> int b, c;
>
> static int
> fn1 (int p1, int p2)
> {
> return p1 > 2147483647 - p2 ? p1 : p1 + p2;
> }
>
> void
> fn2 (void)
> {
> int j;
> a = 30;
> for (; a;)
> for (; c; b = fn1 (j, 1))
> ;
> }
> ...
>
> When compiling the test-case with -Os, just before tail-merge it looks
> as in before.svg.
>
> During tail-merge, it runs into a sigsegv.
>
> What happens is the following:
> - tail-merge decides to merge blocks 4 and 6, and removes block 6.
As pointed out in the PR, blocks 4 and 6 belong to different loops, and
shouldn't be merged.
There is a test 'bb->loop_father->latch == bb' in find_same_succ_bb that
is supposed to keep the two blocks from merging, but the test is not
working for this example because the latch field is not defined (because
the loop has in fact two latches).
This patch prevents blocks from different loops to be merged by testing
for bb->loop_father->num equivalence in tail-merge.
It also removes the unreliable test for 'bb->loop_father->latch == bb'
in find_same_succ_bb.
Bootstrapped and reg-tested on x86_64.
OK for trunk and gcc-[567]-branch?
Thanks,
- Tom
[-- Attachment #2: 0002-Don-t-tail-merge-blocks-from-different-loops.patch --]
[-- Type: text/x-patch, Size: 2462 bytes --]
Don't tail-merge blocks from different loops
2017-06-30 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/81192
* tree-ssa-tail-merge.c (same_succ_hash): Use bb->loop_father->num in
hash.
(same_succ::equal): Don't find bbs to be equal if bb->loop_father->num
differs.
(find_same_succ_bb): Remove obsolete test on bb->loop_father->latch.
* gcc.dg/pr81192.c: Update.
---
gcc/testsuite/gcc.dg/pr81192.c | 2 +-
gcc/tree-ssa-tail-merge.c | 15 ++++++---------
2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/gcc/testsuite/gcc.dg/pr81192.c b/gcc/testsuite/gcc.dg/pr81192.c
index 57eb478..8b3a77a 100644
--- a/gcc/testsuite/gcc.dg/pr81192.c
+++ b/gcc/testsuite/gcc.dg/pr81192.c
@@ -19,4 +19,4 @@ fn2 (void)
;
}
-/* { dg-final { scan-tree-dump-times "(?n)find_duplicates: <bb .*> duplicate of <bb .*>" 1 "pre" } } */
+/* { dg-final { scan-tree-dump-not "(?n)find_duplicates: <bb .*> duplicate of <bb .*>" "pre" } } */
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index bb8a308..0865e86 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -479,6 +479,8 @@ same_succ_hash (const same_succ *e)
hstate.add_int (size);
BB_SIZE (bb) = size;
+ hstate.add_int (bb->loop_father->num);
+
for (i = 0; i < e->succ_flags.length (); ++i)
{
flags = e->succ_flags[i];
@@ -568,6 +570,9 @@ same_succ::equal (const same_succ *e1, const same_succ *e2)
if (BB_SIZE (bb1) != BB_SIZE (bb2))
return 0;
+ if (bb1->loop_father->num != bb2->loop_father->num)
+ return 0;
+
gsi1 = gsi_start_nondebug_bb (bb1);
gsi2 = gsi_start_nondebug_bb (bb2);
gsi_advance_fw_nondebug_nonlocal (&gsi1);
@@ -695,15 +700,7 @@ find_same_succ_bb (basic_block bb, same_succ **same_p)
edge_iterator ei;
edge e;
- if (bb == NULL
- /* Be conservative with loop structure. It's not evident that this test
- is sufficient. Before tail-merge, we've just called
- loop_optimizer_finalize, and LOOPS_MAY_HAVE_MULTIPLE_LATCHES is now
- set, so there's no guarantee that the loop->latch value is still valid.
- But we assume that, since we've forced LOOPS_HAVE_SIMPLE_LATCHES at the
- start of pre, we've kept that property intact throughout pre, and are
- keeping it throughout tail-merge using this test. */
- || bb->loop_father->latch == bb)
+ if (bb == NULL)
return;
bitmap_set_bit (same->bbs, bb->index);
FOR_EACH_EDGE (e, ei, bb->succs)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH, PR69468] Ignore EDGE_{DFS_BACK,EXECUTABLE} in tail-merge
2017-07-02 22:49 ` [PATCH, PR81192] Don't tail-merge blocks from different loops Tom de Vries
@ 2017-07-02 23:12 ` Tom de Vries
2017-07-03 7:04 ` Richard Biener
2017-07-03 7:01 ` [PATCH, PR81192] Don't tail-merge blocks from different loops Richard Biener
1 sibling, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2017-07-02 23:12 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]
[ was: Re: [PATCH, PR81192] Don't tail-merge blocks from different loops ]
On 07/03/2017 12:49 AM, Tom de Vries wrote:
> [ was: Re: [PATCH, PR81192] Fix sigsegv in find_same_succ_bb ]
>
> On 07/03/2017 12:26 AM, Tom de Vries wrote:
>> [ Trying again with before.svg instead of before.pdf ]
>>
>> Hi,
>>
>> consider this test-case:
>> ...
>> unsigned a;
>> int b, c;
>>
>> static int
>> fn1 (int p1, int p2)
>> {
>> return p1 > 2147483647 - p2 ? p1 : p1 + p2;
>> }
>>
>> void
>> fn2 (void)
>> {
>> int j;
>> a = 30;
>> for (; a;)
>> for (; c; b = fn1 (j, 1))
>> ;
>> }
>> ...
>>
>> When compiling the test-case with -Os, just before tail-merge it looks
>> as in before.svg.
>>
>> During tail-merge, it runs into a sigsegv.
>>
>> What happens is the following:
>> - tail-merge decides to merge blocks 4 and 6, and removes block 6.
>
> As pointed out in the PR, blocks 4 and 6 belong to different loops, and
> shouldn't be merged.
>
While working on this PR, I used print_graph_cfg in tail-merge, which I
noticed changes the choice in merging blocks. I tracked this down to:
- mark_dfs_back_edges changing the EDGE_DFS_BACK flag on some edges, and
- tail-merge requiring identical edge flags (apart from the true/false
flags, which are dealt with explicitly).
This is similar to PR69468 which notices the same about EDGE_EXECUTABLE.
This patch allows more tail-merging by ignoring both EDGE_DFS_BACK and
EDGE_EXECUTABLE. Consequently, in this example the two latch bbs (of the
same loop) bb4 and bb7 are merged.
Bootstrapped and reg-tested on x86_64.
OK for trunk?
Thanks,
- Tom
[-- Attachment #2: 0003-Ignore-EDGE_-DFS_BACK-EXECUTABLE-in-tail-merge.patch --]
[-- Type: text/x-patch, Size: 1813 bytes --]
Ignore EDGE_{DFS_BACK,EXECUTABLE} in tail-merge
2017-06-30 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/69468
* tree-ssa-tail-merge.c (ignore_edge_flags): New constant.
(find_same_succ_bb): Handle ignore_edge_flags.
* gcc.dg/pr81192.c: Update.
---
gcc/testsuite/gcc.dg/pr81192.c | 2 +-
gcc/tree-ssa-tail-merge.c | 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/gcc/testsuite/gcc.dg/pr81192.c b/gcc/testsuite/gcc.dg/pr81192.c
index 8b3a77a..57eb478 100644
--- a/gcc/testsuite/gcc.dg/pr81192.c
+++ b/gcc/testsuite/gcc.dg/pr81192.c
@@ -19,4 +19,4 @@ fn2 (void)
;
}
-/* { dg-final { scan-tree-dump-not "(?n)find_duplicates: <bb .*> duplicate of <bb .*>" "pre" } } */
+/* { dg-final { scan-tree-dump-times "(?n)find_duplicates: <bb .*> duplicate of <bb .*>" 1 "pre" } } */
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index 0865e86..6f199de 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -207,6 +207,8 @@ along with GCC; see the file COPYING3. If not see
#include "tree-eh.h"
#include "tree-cfgcleanup.h"
+const int ignore_edge_flags = EDGE_DFS_BACK | EDGE_EXECUTABLE;
+
/* Describes a group of bbs with the same successors. The successor bbs are
cached in succs, and the successor edge flags are cached in succ_flags.
If a bb has the EDGE_TRUE/FALSE_VALUE flags swapped compared to succ_flags,
@@ -707,7 +709,7 @@ find_same_succ_bb (basic_block bb, same_succ **same_p)
{
int index = e->dest->index;
bitmap_set_bit (same->succs, index);
- same_succ_edge_flags[index] = e->flags;
+ same_succ_edge_flags[index] = (e->flags & ~ignore_edge_flags);
}
EXECUTE_IF_SET_IN_BITMAP (same->succs, 0, j, bj)
same->succ_flags.safe_push (same_succ_edge_flags[j]);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, PR81192] Don't tail-merge blocks from different loops
2017-07-02 22:49 ` [PATCH, PR81192] Don't tail-merge blocks from different loops Tom de Vries
2017-07-02 23:12 ` [PATCH, PR69468] Ignore EDGE_{DFS_BACK,EXECUTABLE} in tail-merge Tom de Vries
@ 2017-07-03 7:01 ` Richard Biener
1 sibling, 0 replies; 6+ messages in thread
From: Richard Biener @ 2017-07-03 7:01 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
On Mon, 3 Jul 2017, Tom de Vries wrote:
> [ was: Re: [PATCH, PR81192] Fix sigsegv in find_same_succ_bb ]
>
> On 07/03/2017 12:26 AM, Tom de Vries wrote:
> > [ Trying again with before.svg instead of before.pdf ]
> >
> > Hi,
> >
> > consider this test-case:
> > ...
> > unsigned a;
> > int b, c;
> >
> > static int
> > fn1 (int p1, int p2)
> > {
> > return p1 > 2147483647 - p2 ? p1 : p1 + p2;
> > }
> >
> > void
> > fn2 (void)
> > {
> > int j;
> > a = 30;
> > for (; a;)
> > for (; c; b = fn1 (j, 1))
> > ;
> > }
> > ...
> >
> > When compiling the test-case with -Os, just before tail-merge it looks as in
> > before.svg.
> >
> > During tail-merge, it runs into a sigsegv.
> >
> > What happens is the following:
> > - tail-merge decides to merge blocks 4 and 6, and removes block 6.
>
> As pointed out in the PR, blocks 4 and 6 belong to different loops, and
> shouldn't be merged.
>
> There is a test 'bb->loop_father->latch == bb' in find_same_succ_bb that is
> supposed to keep the two blocks from merging, but the test is not working for
> this example because the latch field is not defined (because the loop has in
> fact two latches).
>
> This patch prevents blocks from different loops to be merged by testing for
> bb->loop_father->num equivalence in tail-merge.
You can compare bb->loop_father directly.
> It also removes the unreliable test for 'bb->loop_father->latch == bb' in
> find_same_succ_bb.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk and gcc-[567]-branch?
Ok for trunk with the above change, as we do not have any wrong-code/ICE
testcase that fails on branches I'd not backport this. Maybe to the
GCC 7 branch but certainly not farther.
Thanks,
Richard.
> Thanks,
> - Tom
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, PR69468] Ignore EDGE_{DFS_BACK,EXECUTABLE} in tail-merge
2017-07-02 23:12 ` [PATCH, PR69468] Ignore EDGE_{DFS_BACK,EXECUTABLE} in tail-merge Tom de Vries
@ 2017-07-03 7:04 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2017-07-03 7:04 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
On Mon, 3 Jul 2017, Tom de Vries wrote:
> [ was: Re: [PATCH, PR81192] Don't tail-merge blocks from different loops ]
>
> On 07/03/2017 12:49 AM, Tom de Vries wrote:
> > [ was: Re: [PATCH, PR81192] Fix sigsegv in find_same_succ_bb ]
> >
> > On 07/03/2017 12:26 AM, Tom de Vries wrote:
> > > [ Trying again with before.svg instead of before.pdf ]
> > >
> > > Hi,
> > >
> > > consider this test-case:
> > > ...
> > > unsigned a;
> > > int b, c;
> > >
> > > static int
> > > fn1 (int p1, int p2)
> > > {
> > > return p1 > 2147483647 - p2 ? p1 : p1 + p2;
> > > }
> > >
> > > void
> > > fn2 (void)
> > > {
> > > int j;
> > > a = 30;
> > > for (; a;)
> > > for (; c; b = fn1 (j, 1))
> > > ;
> > > }
> > > ...
> > >
> > > When compiling the test-case with -Os, just before tail-merge it looks as
> > > in before.svg.
> > >
> > > During tail-merge, it runs into a sigsegv.
> > >
> > > What happens is the following:
> > > - tail-merge decides to merge blocks 4 and 6, and removes block 6.
> >
> > As pointed out in the PR, blocks 4 and 6 belong to different loops, and
> > shouldn't be merged.
> >
>
> While working on this PR, I used print_graph_cfg in tail-merge, which I
> noticed changes the choice in merging blocks. I tracked this down to:
> - mark_dfs_back_edges changing the EDGE_DFS_BACK flag on some edges, and
> - tail-merge requiring identical edge flags (apart from the true/false
> flags, which are dealt with explicitly).
>
> This is similar to PR69468 which notices the same about EDGE_EXECUTABLE.
>
> This patch allows more tail-merging by ignoring both EDGE_DFS_BACK and
> EDGE_EXECUTABLE. Consequently, in this example the two latch bbs (of the same
> loop) bb4 and bb7 are merged.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk?
Ok.
Richard.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, PR81192] Fix sigsegv in find_same_succ_bb
[not found] <2777d355-a59c-a87b-bb64-86cd40eab077@mentor.com>
@ 2017-07-03 6:59 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2017-07-03 6:59 UTC (permalink / raw)
To: Tom de Vries; +Cc: GCC Patches
On Sun, 2 Jul 2017, Tom de Vries wrote:
> Hi,
>
> consider this test-case:
> ...
> unsigned a;
> int b, c;
>
> static int
> fn1 (int p1, int p2)
> {
> return p1 > 2147483647 - p2 ? p1 : p1 + p2;
> }
>
> void
> fn2 (void)
> {
> int j;
> a = 30;
> for (; a;)
> for (; c; b = fn1 (j, 1))
> ;
> }
> ...
>
> When compiling the test-case with -Os, just before tail-merge it looks as in
> before.pdf.
>
> During tail-merge, it runs into a sigsegv.
>
> What happens is the following:
> - tail-merge decides to merge blocks 4 and 6, and removes block 6.
> - bb8, a predecessor of block 6, is marked as member of
> deleted_bb_preds.
> - during update_worklist, same_succ_flush_bb is called for bb8
> - same_succ_flush_bb runs into a sigsegv because
> BB_SAME_SUCC (bb8) == NULL
> - the reason that BB_SAME_SUCC (bb8) == NULL, is because it hit the
> bb->loop_father->latch == bb clause in find_same_succ_bb at the start
> of the tail-merge pass.
>
> This patch fixes the sigsegv by doing an early-out in same_succ_flush_bb if
> BB_SAME_SUCC () == NULL.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk and gcc-[567]-branch?
Ok for trunk and branches. Mind the gcc-6 branch is frozen right now.
Thanks,
Richard.
> Thanks,
> - Tom
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-03 7:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-02 22:26 [PATCH, PR81192] Fix sigsegv in find_same_succ_bb Tom de Vries
2017-07-02 22:49 ` [PATCH, PR81192] Don't tail-merge blocks from different loops Tom de Vries
2017-07-02 23:12 ` [PATCH, PR69468] Ignore EDGE_{DFS_BACK,EXECUTABLE} in tail-merge Tom de Vries
2017-07-03 7:04 ` Richard Biener
2017-07-03 7:01 ` [PATCH, PR81192] Don't tail-merge blocks from different loops Richard Biener
[not found] <2777d355-a59c-a87b-bb64-86cd40eab077@mentor.com>
2017-07-03 6:59 ` [PATCH, PR81192] Fix sigsegv in find_same_succ_bb Richard Biener
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).