This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Prevent cselib substitution of FP, SP, SFP
- From: Carrot Wei <carrot at google dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Marcus Shawcroft <marcus dot shawcroft at arm dot com>, Richard Henderson <rth at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 12 Sep 2012 14:01:22 -0700
- Subject: Re: [PATCH] Prevent cselib substitution of FP, SP, SFP
- References: <4F048AA2.30805@arm.com> <20120213195422.GN18768@tyan-ft48-01.lab.bos.redhat.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