Re: A problem about loop store motion

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Feb 22, 2012 at 2:15 AM, Jiangning Liu <jiangning.liu@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: gcc-owner@gcc.gnu.org [mailto:gcc-owner@gcc.gnu.org] On Behalf Of
>> Richard Guenther
>> Sent: Tuesday, February 21, 2012 6:19 PM
>> To: Jiangning Liu
>> Cc: gcc@gcc.gnu.org
>> Subject: Re: A problem about loop store motion
>>
>> On Tue, Feb 21, 2012 at 10:57 AM, Jiangning Liu <jiangning.liu@arm.com>
>> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> >> Sent: Tuesday, February 21, 2012 5:40 PM
>> >> To: Jiangning Liu
>> >> Cc: gcc@gcc.gnu.org
>> >> Subject: Re: A problem about loop store motion
>> >>
>> >> On Tue, Feb 21, 2012 at 9:54 AM, Jiangning Liu
>> <jiangning.liu@arm.com>
>> >> wrote:
>> >> >> The MEM form is more "canonical", so the loop SM machinery to
>> detect
>> >> >> equality should be adjusted accordingly. ?Alternatively you can
>> >> teach
>> >> >> PRE insertion to strip off the MEM if possible (though
>> >> >> fold_stmt_inplace should
>> >> >> arelady do this if possible).
>> >> >
>> >> > Richard,
>> >> >
>> >> > Thank you! You are right. I noticed on latest trunk the problem in
>> >> PRE was
>> >> > already fixed by invoking fold_stmt_inplace.
>> >> >
>> >> > Unfortunately for this small case, the latest trunk code still
>> can't
>> >> do SM
>> >> > for variable pos, because refs_may_alias_p(*D.4074_10, pos) is
>> true,
>> >> that
>> >> > is, pos has alias with l[pos].
>> >> >
>> >> > I think alias analysis should be able to know they don't have
>> alias
>> >> with
>> >> > each other, unless there is an assignment statement like "l=&pos;".
>> >> >
>> >> > Can alias analysis fix the problem?
>> >>
>> >> The problem is that 'pos' is marked TREE_ADDRESSABLE, so we think
>> >> its address got taken. ?'l' points to any global possibly address-
>> taken
>> >> variable. ?It get's the TREE_ADDRESSABLE flag via PRE insertion
>> which
>> >> re-gimplifies the tree it creates which marks the variable as
>> >> addressable.
>> >>
>> >> I'll have a look.
>> >
>> > Terrific! :-) Could you please let me know when you have a patch to
>> fix
>> > this, because I want to double check the big case, and there might be
>> other
>> > hidden problems?
>>
>> PR52324, I am testing the following patch (in reality the gimplifier
>> should not
>> mark things addressable unless it itself makes them so, but frontends
>> are
>> broken, so we do that ... ugh).
>>
>
> Richard,
>
> Now trunk works for the big case as well. Thanks a lot for your quick fix.
>
> BTW, why can't we fix front-end directly?

front-end?  You mean why we are marking things addressable in the
gimplifier?  Because nobody went down and fixed all latent issues in
the frontends.  Feel free to fix the fallout of

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c      (revision 184460)
+++ gcc/gimplify.c      (working copy)
@@ -4957,8 +4957,6 @@ gimplify_addr_expr (tree *expr_p, gimple
       if (TREE_CODE (op0) == INDIRECT_REF)
        goto do_indirect_ref;

-      mark_addressable (TREE_OPERAND (expr, 0));
-
       /* The FEs may end up building ADDR_EXPRs early on a decl with
         an incomplete type.  Re-build ADDR_EXPRs in canonical form
         here.  */

;)

> Thanks,
> -Jiangning
>
>> Index: gcc/gimplify.c
>> ===================================================================
>> --- gcc/gimplify.c ? ? ?(revision 184428)
>> +++ gcc/gimplify.c ? ? ?(working copy)
>> @@ -7061,15 +7061,20 @@ gimplify_expr (tree *expr_p, gimple_seq
>> ? ? ? ? ? ? ? ret = GS_OK;
>> ? ? ? ? ? ? ? break;
>> ? ? ? ? ? ? }
>> - ? ? ? ? ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
>> post_p,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?is_gimple_mem_ref_addr, fb_rvalue);
>> - ? ? ? ? if (ret == GS_ERROR)
>> - ? ? ? ? ? break;
>> + ? ? ? ? /* Avoid re-gimplifying the address operand if it is already
>> + ? ? ? ? ? ?in suitable form. ?*/
>> + ? ? ? ? if (!is_gimple_mem_ref_addr (TREE_OPERAND (*expr_p, 0)))
>> + ? ? ? ? ? {
>> + ? ? ? ? ? ? ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
>> post_p,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?is_gimple_mem_ref_addr, fb_rvalue);
>> + ? ? ? ? ? ? if (ret == GS_ERROR)
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? }
>> ? ? ? ? ? recalculate_side_effects (*expr_p);
>> ? ? ? ? ? ret = GS_ALL_DONE;
>> ? ? ? ? ? break;
>>
>> - ? ? ? ? /* Constants need not be gimplified. ?*/
>> + ? ? ? /* Constants need not be gimplified. ?*/
>> ? ? ? ? case INTEGER_CST:
>> ? ? ? ? case REAL_CST:
>> ? ? ? ? case FIXED_CST:
>
>
>
>