RE: A problem about loop store motion

["Jiangning Liu" <jiangning dot liu at arm dot com>]

> -----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?

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: