This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]