Re: [PATCH] Prevent cselib substitution of FP, SP, SFP

[Carrot Wei <carrot at google dot com>]

Hi Jakub

The same problem also affects gcc4.6,
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54398. Could this be
ported to 4.6 branch?

thanks
Carrot

On Mon, Feb 13, 2012 at 11:54 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Jan 04, 2012 at 05:21:38PM +0000, Marcus Shawcroft wrote:
> > Alias analysis by DSE based on CSELIB expansion assumes that
> > references to the stack frame from different base registers (ie FP, SP)
> > never alias.
> >
> > The comment block in cselib explains that cselib does not allow
> > substitution of FP, SP or SFP specifically in order not to break DSE.
>
> Looks reasonable, appart from coding style (no spaces around -> and
> no {} around return p->loc;), I just wonder if having a separate
> loop in expand_loc just for this isn't too expensive.  On sane targets
> IMHO hard frame pointer in the prologue should be initialized from sp, not
> the other way around, thus hard frame pointer based VALUEs should have
> hard frame pointer earlier in the locs list (when there is
> hfp = sp (+ optionally some const)
> insn, we first cselib_lookup_from_insn the rhs and add to locs
> of the new VALUE (plus (VALUE of sp) (const_int)), then process the
> lhs and add it to locs, moving the plus to locs->next).
> So I think the following patch could be enough (bootstrapped/regtested
> on x86_64-linux and i686-linux).
> There is AVR though, which has really weirdo prologue - PR50063,
> but I think it should just use UNSPEC for that or something similar,
> setting sp from hfp seems unnecessary and especially for values with long
> locs chains could make cselib more expensive.
>
> Richard, what do you think about this?
>
> 2012-02-13  Jakub Jelinek  <jakub@redhat.com>
>
>         * cselib.c (expand_loc): Return sp, fp, hfp or cfa base reg right
>         away if seen.
>
> --- gcc/cselib.c.jj     2012-02-13 11:07:15.000000000 +0100
> +++ gcc/cselib.c        2012-02-13 18:15:17.531776145 +0100
> @@ -1372,8 +1372,18 @@ expand_loc (struct elt_loc_list *p, stru
>    unsigned int regno = UINT_MAX;
>    struct elt_loc_list *p_in = p;
>
> -  for (; p; p = p -> next)
> +  for (; p; p = p->next)
>      {
> +      /* Return these right away to avoid returning stack pointer based
> +        expressions for frame pointer and vice versa, which is something
> +        that would confuse DSE.  See the comment in
> cselib_expand_value_rtx_1
> +        for more details.  */
> +      if (REG_P (p->loc)
> +         && (REGNO (p->loc) == STACK_POINTER_REGNUM
> +             || REGNO (p->loc) == FRAME_POINTER_REGNUM
> +             || REGNO (p->loc) == HARD_FRAME_POINTER_REGNUM
> +             || REGNO (p->loc) == cfa_base_preserved_regno))
> +       return p->loc;
>        /* Avoid infinite recursion trying to expand a reg into a
>          the same reg.  */
>        if ((REG_P (p->loc))
>
>
>         Jakub