diff mbox

Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2)

Message ID 20121119203954.GM2315@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Nov. 19, 2012, 8:39 p.m. UTC 
On Mon, Nov 19, 2012 at 08:56:22AM -0800, Richard Henderson wrote:
> On 2012-11-18 23:53, Jakub Jelinek wrote:
> > I'd prefer to only invalidate the stack pointer on the first assignment
> > to the hard pointer.  If the cselib link between sp and hfp is already
> > broken, invalidating sp will only result in worse code.  Dunno if there
> > are any targets that adjust the hard frame pointer after it has been set
> > once or similar.
> 
> I'm not aware of such a target.  Though I did not audit them now.
> 
> > Perhaps we could walk here CSELIB_VAL_PTR (hfpval)->locs here, and look
> > if any rtls in there have find_base_term (x->loc) == find_base_term
> > (stack_pointer_rtx), and only if yes, invalidate (and guard it by the
> > modified_in_p test).
> 
> Sounds plausible.
> 
> > BTW, var-tracking.c uses a similar test.
> 
> Ouch.  Where is that?

Here is an updated patch, bootstrapped/regtested on x86_64-linux and
i686-linux, ok for trunk?

2012-11-19  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/54921
	* cselib.h (fp_setter_insn): New prototype.
	* cselib.c (fp_setter_insn): New function.
	(cselib_process_insn): If frame_pointer_needed,
	call cselib_invalidate_rtx (stack_pointer_rtx) after
	processing a frame pointer setter.
	* var-tracking.c (fp_setter): Removed.
	(vt_initialize): Use fp_setter_insn instead of fp_setter.

	* gcc.dg/pr54921.c: New test.



	Jakub

Comments

Richard Henderson Nov. 19, 2012, 9:42 p.m. UTC | #1 
On 11/19/2012 12:39 PM, Jakub Jelinek wrote:
> +  if (reload_completed
> +      && frame_pointer_needed
> +      && RTX_FRAME_RELATED_P (insn)
> +      && fp_setter_insn (insn))
> +    cselib_invalidate_rtx (stack_pointer_rtx);
...
>  		  if (fp_cfa_offset != -1
>  		      && hard_frame_pointer_adjustment == -1
>  		      && RTX_FRAME_RELATED_P (insn)
> -		      && fp_setter (insn))
> +		      && fp_setter_insn (insn))

I thought we talked about moving the RTX_FRAME_RELATED_P test inside
the function?  Since it's already there anyway...


r~
Jakub Jelinek Nov. 19, 2012, 9:45 p.m. UTC | #2 
On Mon, Nov 19, 2012 at 01:42:06PM -0800, Richard Henderson wrote:
> On 11/19/2012 12:39 PM, Jakub Jelinek wrote:
> > +  if (reload_completed
> > +      && frame_pointer_needed
> > +      && RTX_FRAME_RELATED_P (insn)
> > +      && fp_setter_insn (insn))
> > +    cselib_invalidate_rtx (stack_pointer_rtx);
> ...
> >  		  if (fp_cfa_offset != -1
> >  		      && hard_frame_pointer_adjustment == -1
> >  		      && RTX_FRAME_RELATED_P (insn)
> > -		      && fp_setter (insn))
> > +		      && fp_setter_insn (insn))
> 
> I thought we talked about moving the RTX_FRAME_RELATED_P test inside
> the function?  Since it's already there anyway...

Ah, forgot to remove it in the callers.  Will do.  Is it ok with that change?

	Jakub
Richard Henderson Nov. 19, 2012, 9:58 p.m. UTC | #3 
On 11/19/2012 01:45 PM, Jakub Jelinek wrote:
> Ah, forgot to remove it in the callers.  Will do.  Is it ok with that change?

Yes.


r~
diff mbox

Patch

--- gcc/cselib.h.jj	2012-10-16 13:20:25.000000000 +0200
+++ gcc/cselib.h	2012-11-19 19:02:27.768645544 +0100
@@ -78,6 +78,7 @@  extern void cselib_init (int);
 extern void cselib_clear_table (void);
 extern void cselib_finish (void);
 extern void cselib_process_insn (rtx);
+extern bool fp_setter_insn (rtx);
 extern enum machine_mode cselib_reg_set_mode (const_rtx);
 extern int rtx_equal_for_cselib_p (rtx, rtx);
 extern int references_value_p (const_rtx, int);
--- gcc/cselib.c.jj	2012-11-12 11:23:02.579098220 +0100
+++ gcc/cselib.c	2012-11-19 19:16:25.406747280 +0100
@@ -2593,6 +2593,28 @@  cselib_record_sets (rtx insn)
     }
 }
 
+/* Return true if INSN in the prologue initializes hard_frame_pointer_rtx.  */
+
+bool
+fp_setter_insn (rtx insn)
+{
+  rtx expr, pat = NULL_RTX;
+
+  if (!RTX_FRAME_RELATED_P (insn))
+    return false;
+
+  expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX);
+  if (expr)
+    pat = XEXP (expr, 0);
+  if (!modified_in_p (hard_frame_pointer_rtx, pat ? pat : insn))
+    return false;
+
+  /* Don't return true for frame pointer restores in the epilogue.  */
+  if (find_reg_note (insn, REG_CFA_RESTORE, hard_frame_pointer_rtx))
+    return false;
+  return true;
+}
+
 /* Record the effects of INSN.  */
 
 void
@@ -2651,6 +2673,15 @@  cselib_process_insn (rtx insn)
       if (GET_CODE (XEXP (x, 0)) == CLOBBER)
 	cselib_invalidate_rtx (XEXP (XEXP (x, 0), 0));
 
+  /* On setter of the hard frame pointer if frame_pointer_needed,
+     invalidate stack_pointer_rtx, so that sp and {,h}fp based
+     VALUEs are distinct.  */
+  if (reload_completed
+      && frame_pointer_needed
+      && RTX_FRAME_RELATED_P (insn)
+      && fp_setter_insn (insn))
+    cselib_invalidate_rtx (stack_pointer_rtx);
+
   cselib_current_insn = NULL_RTX;
 
   if (n_useless_values > MAX_USELESS_VALUES
--- gcc/var-tracking.c.jj	2012-11-19 14:41:26.000000000 +0100
+++ gcc/var-tracking.c	2012-11-19 18:59:13.638780830 +0100
@@ -9522,40 +9522,6 @@  vt_add_function_parameters (void)
     }
 }
 
-/* Return true if INSN in the prologue initializes hard_frame_pointer_rtx.  */
-
-static bool
-fp_setter (rtx insn)
-{
-  rtx pat = PATTERN (insn);
-  if (RTX_FRAME_RELATED_P (insn))
-    {
-      rtx expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX);
-      if (expr)
-	pat = XEXP (expr, 0);
-    }
-  if (GET_CODE (pat) == SET)
-    {
-      if (SET_DEST (pat) != hard_frame_pointer_rtx)
-	return false;
-    }
-  else if (GET_CODE (pat) == PARALLEL)
-    {
-      int i;
-      for (i = XVECLEN (pat, 0) - 1; i >= 0; i--)
-	if (GET_CODE (XVECEXP (pat, 0, i)) == SET
-	    && SET_DEST (XVECEXP (pat, 0, i)) == hard_frame_pointer_rtx)
-	  break;
-      if (i < 0)
-	return false;
-    }
-  else
-    return false;
-  if (find_reg_note (insn, REG_CFA_RESTORE, hard_frame_pointer_rtx))
-    return false;
-  return true;
-}
-
 /* Initialize cfa_base_rtx, create a preserved VALUE for it and
    ensure it isn't flushed during cselib_reset_table.
    Can be called only if frame_pointer_rtx resp. arg_pointer_rtx
@@ -9860,7 +9826,7 @@  vt_initialize (void)
 		  if (fp_cfa_offset != -1
 		      && hard_frame_pointer_adjustment == -1
 		      && RTX_FRAME_RELATED_P (insn)
-		      && fp_setter (insn))
+		      && fp_setter_insn (insn))
 		    {
 		      vt_init_cfa_base ();
 		      hard_frame_pointer_adjustment = fp_cfa_offset;
--- gcc/testsuite/gcc.dg/pr54921.c.jj	2012-11-19 18:52:50.016031798 +0100
+++ gcc/testsuite/gcc.dg/pr54921.c	2012-11-19 18:52:50.016031798 +0100
@@ -0,0 +1,32 @@ 
+/* PR rtl-optimization/54921 */
+/* { dg-do run } */
+/* { dg-options "-Os -fno-omit-frame-pointer -fsched2-use-superblocks -ftree-slp-vectorize" } */
+/* { dg-additional-options "-fstack-protector" { target fstack_protector } } */
+
+struct A
+{
+  int a;
+  char b[32];
+} a, b;
+
+__attribute__((noinline, noclone))
+struct A
+bar (int x)
+{
+  struct A r;
+  static int n;
+  r.a = ++n;
+  __builtin_memset (r.b, 0, sizeof (r.b));
+  r.b[0] = x;
+  return r;
+}
+
+int
+main ()
+{
+  a = bar (3);
+  b = bar (4);
+  if (a.a != 1 || a.b[0] != 3 || b.a != 2 || b.b[0] != 4)
+    __builtin_abort ();
+  return 0;
+}