This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix for PR51879 - Missed tail merging with non-const/pure calls
- From: Richard Guenther <richard dot guenther at gmail dot com>
- To: Tom de Vries <Tom_deVries at mentor dot com>
- Cc: Jakub Jelinek <jakub at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 3 May 2012 12:21:27 +0200
- Subject: Re: [PATCH] Fix for PR51879 - Missed tail merging with non-const/pure calls
- References: <4F1DD0CF.8010808@mentor.com> <CAFiYyc28OogUwtJyouGTLv9VcZ+Z=BsupY2HbJXFq4WZdNfjtg@mail.gmail.com> <4F230B0F.7080107@mentor.com> <4F89268B.6080007@mentor.com> <CAFiYyc1rThWX-0MRpAuZuOhXNpJK-tk_zpMX3oQignM0oDcp=Q@mail.gmail.com> <4F9718E9.5080903@mentor.com> <CAFiYyc1vYsr_aj6wETaj=KeJUnQABNEU5w+NUCpdxrMaf3BurQ@mail.gmail.com> <4F987310.5000603@mentor.com> <CAFiYyc3LD-hs0xPQ5qszbcmyo-NWPN840PeCtCcd5HenDm+EVw@mail.gmail.com> <4F9A3A90.4050608@mentor.com> <CAFiYyc2CHdj6n8LtJbwR5T7E1c5Nv6_M+f7LsYhPYxHPtzKa0A@mail.gmail.com> <4FA13F5F.2010504@mentor.com>
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.