Message ID | 4FA13F5F.2010504@mentor.com |
---|---|
State | New |
Headers | show |
On Wed, May 2, 2012 at 4:06 PM, Tom de Vries <Tom_deVries@mentor.com> wrote: > On 27/04/12 11:01, Richard Guenther wrote: > <SNIP> >>>>>> I see you do not handle > <SNIP> >>>>>> struct S { int i; }; >>>>>> struct S foo (void); >>>>>> struct S bar (void) >>>>>> { >>>>>> struct S s1, s2; >>>>>> if (...) >>>>>> s = foo (); >>>>>> else >>>>>> s = foo (); >>>>>> >>>>>> because the calls have a LHS that is not an SSA name. >>>>> >>>>> Indeed, the gvn patch handles this example conservatively, and tree-tail-merge >>>>> fails to optimize this test-case: >>>>> ... >>>>> struct S { int i; }; >>>>> extern struct S foo (void); >>>>> extern int foo2 (void); >>>>> struct S s; >>>>> int bar (int c) { >>>>> int r; >>>>> if (c) >>>>> { >>>>> s = foo (); >>>>> r = foo2 (); >>>>> } >>>>> else >>>>> { >>>>> s = foo (); >>>>> r = foo2 (); >>>>> } >>>>> return r; >>>>> } >>>>> ... >>>>> >>>>> A todo. >>>>> > <SNIP> >>>>> bootstrapped and reg-tested on x86_64 (ada inclusive). >>>>> >>>>> Is this patch ok, or is the todo required? >>>> >>>> No, you can followup with that. >>>> > > Richard, > > here is the follow-up patch, which adds value numbering of a call for which the > lhs is not an SSA_NAME. > > The only thing I ended up using from the patch in > http://gcc.gnu.org/ml/gcc-patches/2012-01/msg01731.html was the idea of using > MODIFY_EXPR. > > I don't include any handling of MODIFY_EXPR in create_component_ref_by_pieces_1 > because I don't think it will trigger with PRE. > > bootstrapped and reg-tested on x86_64. > > Ok for trunk? Hmm, I wonder why if (!gimple_call_internal_p (stmt) && (gimple_call_flags (stmt) & (ECF_PURE | ECF_CONST) /* If the call has side effects, subsequent calls won't have the same incoming vuse, so it's save to assume equality. */ || gimple_has_side_effects (stmt))) works - I realize you added the gimple_has_side_effects () call - but if you consider ECF_LOOPING_CONST_OR_PURE functions, which have no VDEF, then it's odd how the comment applies. And together both tests turn out to let all calls pass. + tree lhs = gimple_call_lhs (call); + + if (lhs && TREE_CODE (lhs) != SSA_NAME) + { + memset (&temp, 0, sizeof (temp)); + temp.opcode = MODIFY_EXPR; + temp.type = TREE_TYPE (lhs); + temp.op0 = lhs; + temp.off = -1; + VEC_safe_push (vn_reference_op_s, heap, *result, &temp); + } this deserves a comment - you are adding the extra operand solely for the purpose of hashing. You are also not doing a good job identifying common calls. Consider if () *p = foo (); else *q = foo (); where p and q are value-numbered the same. You fail to properly commonize the blocks. That is because valueization of the ops of the call does not work for arbitrarily complex operands - see how we handle call operands. Instead you should probably use copy_reference_ops_from_ref on the lhs, similar to call operands. Using MODIFY_EXPR as toplevel code for the vn_reference is going to indeed disable PRE for them, likewise any other call handling in VN. Otherwise the idea looks ok - can you change the patch like above and add a testcase with an equal-VNed indirect store? Thanks, Richard. > Thanks, > - Tom > > 2012-05-02 Tom de Vries <tom@codesourcery.com> > > * tree-ssa-sccvn.c (copy_reference_ops_from_call) > (visit_reference_op_call): Handle case that lhs is not an SSA_NAME. > (visit_use): Call visit_reference_op_call for calls with lhs that is not > an SSA_NAME. > > * gcc.dg/pr51879-16.c: New test. > * gcc.dg/pr51879-17.c: Same.
Index: gcc/tree-ssa-sccvn.c =================================================================== --- gcc/tree-ssa-sccvn.c (revision 186907) +++ gcc/tree-ssa-sccvn.c (working copy) @@ -942,6 +947,17 @@ copy_reference_ops_from_call (gimple cal { vn_reference_op_s temp; unsigned i; + tree lhs = gimple_call_lhs (call); + + if (lhs && TREE_CODE (lhs) != SSA_NAME) + { + memset (&temp, 0, sizeof (temp)); + temp.opcode = MODIFY_EXPR; + temp.type = TREE_TYPE (lhs); + temp.op0 = lhs; + temp.off = -1; + VEC_safe_push (vn_reference_op_s, heap, *result, &temp); + } /* Copy the type, opcode, function being called and static chain. */ memset (&temp, 0, sizeof (temp)); @@ -2624,6 +2641,9 @@ visit_reference_op_call (tree lhs, gimpl tree vuse = gimple_vuse (stmt); tree vdef = gimple_vdef (stmt); + if (lhs && TREE_CODE (lhs) != SSA_NAME) + lhs = NULL_TREE; + vr1.vuse = vuse ? SSA_VAL (vuse) : NULL_TREE; vr1.operands = valueize_shared_reference_ops_from_call (stmt); vr1.type = gimple_expr_type (stmt); @@ -3410,9 +3432,7 @@ visit_use (tree use) /* If the call has side effects, subsequent calls won't have the same incoming vuse, so it's save to assume equality. */ - || gimple_has_side_effects (stmt)) - && ((lhs && TREE_CODE (lhs) == SSA_NAME) - || (!lhs && gimple_vdef (stmt)))) + || gimple_has_side_effects (stmt))) { changed = visit_reference_op_call (lhs, stmt); } Index: gcc/testsuite/gcc.dg/pr51879-16.c =================================================================== --- /dev/null (new file) +++ gcc/testsuite/gcc.dg/pr51879-16.c (revision 0) @@ -0,0 +1,32 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-pre" } */ + +struct S { + int i; +}; + +extern struct S foo (void); +extern int foo2 (void); + +struct S s; + +int bar (int c) { + int r; + + if (c) + { + s = foo (); + r = foo2 (); + } + else + { + s = foo (); + r = foo2 (); + } + + return r; +} + +/* { dg-final { scan-tree-dump-times "foo \\(" 1 "pre"} } */ +/* { dg-final { scan-tree-dump-times "foo2 \\(" 1 "pre"} } */ +/* { dg-final { cleanup-tree-dump "pre" } } */ Index: gcc/testsuite/gcc.dg/pr51879-17.c =================================================================== --- /dev/null (new file) +++ gcc/testsuite/gcc.dg/pr51879-17.c (revision 0) @@ -0,0 +1,32 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-pre" } */ + +struct S { + int i; +}; + +extern struct S foo (void); +extern int foo2 (void); + +struct S s, s2; + +int bar (int c) { + int r; + + if (c) + { + s = foo (); + r = foo2 (); + } + else + { + s2 = foo (); + r = foo2 (); + } + + return r; +} + +/* { dg-final { scan-tree-dump-times "foo \\(" 2 "pre"} } */ +/* { dg-final { scan-tree-dump-times "foo2 \\(" 2 "pre"} } */ +/* { dg-final { cleanup-tree-dump "pre" } } */