On Thu, Jul 5, 2012 at 6:44 PM, Tom de Vries <tom_devr...@mentor.com> wrote:
> On 03/05/12 12:21, Richard Guenther wrote:
>> 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.
>>
>
> Richard,
>
> You're right, this is not correct. The test for gimple_has_side_effect should
> be
> a test for gimple_vdef.
> A ECF_LOOPING_CONST_OR_PURE function will be rejected by the updated
> condition.
>
> I fixed this in the patch, and added comments describing both the const/pure
> clause, and the vdef clause.
>
> I also removed the comment 'We should handle stores from calls' since this
> patch
> implements that.
>
>> + 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
>
> Done.
>
>> - 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.
>>
>
> If p and q are value numbered, it means they're SSA_NAMEs, and that means
> they're not handled by this patch which is only about handling calls for which
> the lhs is not an SSA_NAME.
>
> This example is handled by the patch I posted for pr52009. I reposted the
> patch
> and added this test-case
> (http://gcc.gnu.org/ml/gcc-patches/2012-07/msg00155.html).
>
> So I'm not using copy_reference_ops_from_ref on the lhs, since it's not an
> SSA_NAME.
>
>> 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?
>>
>
> I updated the patch as indicated in my comments, and added the test-case to
> the
> patch for pr52009.
>
> Bootstrapped and reg-tested on x86_64 (ada inclusive).
>
> OK for trunk?
Ok.
Thanks,
Richard.
> Thanks,
> - Tom
>
> 2012-07-05 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): Also call visit_reference_op_call for calls with a vdef.
>
> * gcc.dg/pr51879-16.c: New test.
> * gcc.dg/pr51879-17.c: Same.
