| Message ID | 546B7A81.6030508@mentor.com |
|---|---|
| State | New |
| Headers | show |
On 11/18/14 09:57, Tom de Vries wrote: > Richard, > > this (trunk) patch fixes PR62167. > > The patch fixes a problem that triggers with the test-case on the 4.8 > branch, when tail-merge makes a dead type-unsafe load alive. > > I'm not able to reproduce this bug on 4.9 and trunk with the same > test-case. On those branches, the tail-merge already does not happen. > > The reason for the difference is as follows: With 4.8 the two phi > arguments of the phi in the tail block are value-numbered identically: > ... > SCC consists of: p_14 > Value numbering p_14 stmt = p_14 = MEM[(struct head *)_13].first; > Setting value number of p_14 to p_14 (changed) > > SCC consists of: p_15 > Value numbering p_15 stmt = p_15 = _13->next; > Setting value number of p_15 to p_14 (changed) > ... > > With 4.9 (and trunk), that's not the case: > ... > SCC consists of: p_14 > Value numbering p_14 stmt = p_14 = MEM[(struct head *)&heads][k.1_9].first; > Setting value number of p_14 to p_14 (changed) > > SCC consists of: p_15 > Value numbering p_15 stmt = p_15 = _13->next; > Setting value number of p_15 to p_15 (changed) > ... > > I'm not sure the bug triggers on trunk and 4.9, but I see no reason why > it could not trigger, so I'd prefer to apply the patch to 4.9 and trunk > as well. > > The patch introduces an xfail for pr51879-12.c. I can follow up with a > patch to improve upon that, but I think that's better limited to trunk > only. > > Bootstrapped and reg-tested on x86_64/trunk. So instead of simply disabling for anything with virtual operands, shouldn't instead we be comparing the virtual operands and alias information? Seems to me that if we did that properly, this would "just work". Right? jeff
On Tue, 18 Nov 2014, Tom de Vries wrote: > Richard, > > this (trunk) patch fixes PR62167. > > The patch fixes a problem that triggers with the test-case on the 4.8 branch, > when tail-merge makes a dead type-unsafe load alive. > > I'm not able to reproduce this bug on 4.9 and trunk with the same test-case. > On those branches, the tail-merge already does not happen. > > The reason for the difference is as follows: With 4.8 the two phi arguments of > the phi in the tail block are value-numbered identically: > ... > SCC consists of: p_14 > Value numbering p_14 stmt = p_14 = MEM[(struct head *)_13].first; > Setting value number of p_14 to p_14 (changed) > > SCC consists of: p_15 > Value numbering p_15 stmt = p_15 = _13->next; > Setting value number of p_15 to p_14 (changed) > ... > > With 4.9 (and trunk), that's not the case: > ... > SCC consists of: p_14 > Value numbering p_14 stmt = p_14 = MEM[(struct head *)&heads][k.1_9].first; > Setting value number of p_14 to p_14 (changed) > > SCC consists of: p_15 > Value numbering p_15 stmt = p_15 = _13->next; > Setting value number of p_15 to p_15 (changed) > ... > > I'm not sure the bug triggers on trunk and 4.9, but I see no reason why it > could not trigger, so I'd prefer to apply the patch to 4.9 and trunk as well. > > The patch introduces an xfail for pr51879-12.c. I can follow up with a patch > to improve upon that, but I think that's better limited to trunk only. Yeah. > Bootstrapped and reg-tested on x86_64/trunk. > > OK for trunk/stage3, 4.8, 4.9? Ok. Note that this goes well with disabling the (re-)use of value-numbering in tail-merging (which causes me quite some headaches...). On trunk it shouldn't be necessary as much as it was earlier as we now fully propagate constants and copies during FRE/PRE elimination. Thanks, Richard.
On Tue, 18 Nov 2014, Jeff Law wrote: > On 11/18/14 09:57, Tom de Vries wrote: > > Richard, > > > > this (trunk) patch fixes PR62167. > > > > The patch fixes a problem that triggers with the test-case on the 4.8 > > branch, when tail-merge makes a dead type-unsafe load alive. > > > > I'm not able to reproduce this bug on 4.9 and trunk with the same > > test-case. On those branches, the tail-merge already does not happen. > > > > The reason for the difference is as follows: With 4.8 the two phi > > arguments of the phi in the tail block are value-numbered identically: > > ... > > SCC consists of: p_14 > > Value numbering p_14 stmt = p_14 = MEM[(struct head *)_13].first; > > Setting value number of p_14 to p_14 (changed) > > > > SCC consists of: p_15 > > Value numbering p_15 stmt = p_15 = _13->next; > > Setting value number of p_15 to p_14 (changed) > > ... > > > > With 4.9 (and trunk), that's not the case: > > ... > > SCC consists of: p_14 > > Value numbering p_14 stmt = p_14 = MEM[(struct head *)&heads][k.1_9].first; > > Setting value number of p_14 to p_14 (changed) > > > > SCC consists of: p_15 > > Value numbering p_15 stmt = p_15 = _13->next; > > Setting value number of p_15 to p_15 (changed) > > ... > > > > I'm not sure the bug triggers on trunk and 4.9, but I see no reason why > > it could not trigger, so I'd prefer to apply the patch to 4.9 and trunk > > as well. > > > > The patch introduces an xfail for pr51879-12.c. I can follow up with a > > patch to improve upon that, but I think that's better limited to trunk > > only. > > > > Bootstrapped and reg-tested on x86_64/trunk. > So instead of simply disabling for anything with virtual operands, shouldn't > instead we be comparing the virtual operands and alias information? Seems to > me that if we did that properly, this would "just work". Right? But that would simply use operand_equal_p () .... Richard.
2014-11-17 Tom de Vries <tom@codesourcery.com> PR tree-optimization/62167 * tree-ssa-tail-merge.c (stmt_local_def): Handle statements with vuse conservatively. (gimple_equal_p): Don't use vn_valueize to compare for lhs equality of assigns. * gcc.dg/pr51879-12.c: Add xfails. * gcc.dg/pr62167-run.c: New test. * gcc.dg/pr62167.c: New test. --- gcc/testsuite/gcc.dg/pr51879-12.c | 4 +-- gcc/testsuite/gcc.dg/pr62167-run.c | 47 +++++++++++++++++++++++++++++++++++ gcc/testsuite/gcc.dg/pr62167.c | 50 ++++++++++++++++++++++++++++++++++++++ gcc/tree-ssa-tail-merge.c | 6 +++-- 4 files changed, 103 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr62167-run.c create mode 100644 gcc/testsuite/gcc.dg/pr62167.c diff --git a/gcc/testsuite/gcc.dg/pr51879-12.c b/gcc/testsuite/gcc.dg/pr51879-12.c index 8126505..85e2687 100644 --- a/gcc/testsuite/gcc.dg/pr51879-12.c +++ b/gcc/testsuite/gcc.dg/pr51879-12.c @@ -24,6 +24,6 @@ foo (int y) baz (a); } -/* { dg-final { scan-tree-dump-times "bar \\(" 1 "pre"} } */ -/* { dg-final { scan-tree-dump-times "bar2 \\(" 1 "pre"} } */ +/* { dg-final { scan-tree-dump-times "bar \\(" 1 "pre" { xfail *-*-* } } } */ +/* { dg-final { scan-tree-dump-times "bar2 \\(" 1 "pre" { xfail *-*-* } } } */ /* { dg-final { cleanup-tree-dump "pre" } } */ diff --git a/gcc/testsuite/gcc.dg/pr62167-run.c b/gcc/testsuite/gcc.dg/pr62167-run.c new file mode 100644 index 0000000..37214a3 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr62167-run.c @@ -0,0 +1,47 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -ftree-tail-merge" } */ + +struct node +{ + struct node *next; + struct node *prev; +}; + +struct node node; + +struct head +{ + struct node *first; +}; + +struct head heads[5]; + +int k = 2; + +struct head *head = &heads[2]; + +int +main () +{ + struct node *p; + + node.next = (void*)0; + + node.prev = (void *)head; + + head->first = &node; + + struct node *n = head->first; + + struct head *h = &heads[k]; + + heads[2].first = n->next; + + if ((void*)n->prev == (void *)h) + p = h->first; + else + /* Dead tbaa-unsafe load from ((struct node *)&heads[2])->next. */ + p = n->prev->next; + + return !(p == (void*)0); +} diff --git a/gcc/testsuite/gcc.dg/pr62167.c b/gcc/testsuite/gcc.dg/pr62167.c new file mode 100644 index 0000000..f8c31a0 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr62167.c @@ -0,0 +1,50 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -ftree-tail-merge -fdump-tree-pre" } */ + +struct node +{ + struct node *next; + struct node *prev; +}; + +struct node node; + +struct head +{ + struct node *first; +}; + +struct head heads[5]; + +int k = 2; + +struct head *head = &heads[2]; + +int +main () +{ + struct node *p; + + node.next = (void*)0; + + node.prev = (void *)head; + + head->first = &node; + + struct node *n = head->first; + + struct head *h = &heads[k]; + + heads[2].first = n->next; + + if ((void*)n->prev == (void *)h) + p = h->first; + else + /* Dead tbaa-unsafe load from ((struct node *)&heads[2])->next. */ + p = n->prev->next; + + return !(p == (void*)0); +} + +/* { dg-final { scan-tree-dump-not "Removing basic block" "pre"} } */ +/* { dg-final { cleanup-tree-dump "pre" } } */ diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c index 303bd5e..1651985 100644 --- a/gcc/tree-ssa-tail-merge.c +++ b/gcc/tree-ssa-tail-merge.c @@ -326,7 +326,8 @@ stmt_local_def (gimple stmt) if (gimple_vdef (stmt) != NULL_TREE || gimple_has_side_effects (stmt) - || gimple_could_trap_p_1 (stmt, false, false)) + || gimple_could_trap_p_1 (stmt, false, false) + || gimple_vuse (stmt) != NULL_TREE) return false; def_p = SINGLE_SSA_DEF_OPERAND (stmt, SSA_OP_DEF); @@ -1175,7 +1176,8 @@ gimple_equal_p (same_succ same_succ, gimple s1, gimple s2) gimple_assign_rhs1 (s2))); else if (TREE_CODE (lhs1) == SSA_NAME && TREE_CODE (lhs2) == SSA_NAME) - return vn_valueize (lhs1) == vn_valueize (lhs2); + return operand_equal_p (gimple_assign_rhs1 (s1), + gimple_assign_rhs1 (s2), 0); return false; case GIMPLE_COND: -- 1.9.1