On Wed, May 2, 2012 at 4:06 PM, Tom de Vries <tom_devr...@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 <t...@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.