diff mbox

Correctly handle stack arguments of sibling calls in DSE pass

Message ID BLU436-SMTP1810694A69556EE9F0EC80197670@phx.gbl
State New
Headers show

Commit Message

John David Anglin Dec. 7, 2014, 7:10 p.m. UTC 
On 1-Dec-14, at 11:57 AM, Jeff Law wrote:

> Prior to reload (ie, in DSE1) there's a bit of magic in that we do  
> not set frame_read on call insns.  That may in fact be wrong and  
> possibly the source of the problem.
>
> /* This field is only used for the processing of const functions.
>     These functions cannot read memory, but they can read the stack
>     because that is where they may get their parms.  We need to be
>     this conservative because, like the store motion pass, we don't
>     consider CALL_INSN_FUNCTION_USAGE when processing call insns.
>     Moreover, we need to distinguish two cases:
>     1. Before reload (register elimination), the stores related to
>        outgoing arguments are stack pointer based and thus deemed
>        of non-constant base in this pass.  This requires special
>        handling but also means that the frame pointer based stores
>        need not be killed upon encountering a const function call.
>     2. After reload, the stores related to outgoing arguments can be
>        either stack pointer or hard frame pointer based.  This means
>        that we have no other choice than also killing all the frame
>        pointer based stores upon encountering a const function call.
>     This field is set after reload for const function calls.  Having
>     this set is less severe than a wild read, it just means that all
>     the frame related stores are killed rather than all the stores.   
> */
>  bool frame_read;
>
>
> As a test, what happens if we change:
>
>
>          /* See the head comment of the frame_read field.  */
>          if (reload_completed)
>            insn_info->frame_read = true;
>
> To do unconditionally set frame_read?  Or if we don't want to get  
> that drastic,
>
> if (reload_completed || SIBLING_CALL_P (insn))
>  insn_info->frame_read = true;


The attached patch uses your suggestion above.  It also doesn't kill  
stack based stores
before reload when we have a sibling call.

Sibling calls use the incoming argument pointer to store arguments for  
the sibcall.  Typically,
the argument pointer and the frame pointer are related, but not on  
hppa64 where sibcalls are
disabled.  So, I have to wonder if the first approach wasn't better.

The change updates the comment for the frame_read field to mention  
sibling calls and
adds a testcase.

Tested on hppa-unknown-linux-gnu, hppa2.0w-hp-hpux11.11, hppa64-hp- 
hpux11.11
with no observed regressions.

Bootstrap tested on on i686-unknown-linux-gnu.

Okay?

Dave
--
John David Anglin	dave.anglin@bell.net
2014-12-07  John David Anglin  <danglin@gcc.gnu.org>

	PR target/55023
	* dse.c (frame_read): Update comment.
	(scan_insn): Set insn_info->frame_read to true for sibling calls.
	Don't remove stack pointer based stores for sibling calls before
	reload.
diff mbox

Patch

Index: gcc/dse.c
===================================================================
--- gcc/dse.c	(revision 218467)
+++ gcc/dse.c	(working copy)
@@ -363,7 +363,8 @@ 
      consider CALL_INSN_FUNCTION_USAGE when processing call insns.
      Moreover, we need to distinguish two cases:
      1. Before reload (register elimination), the stores related to
-	outgoing arguments are stack pointer based and thus deemed
+	outgoing arguments are stack pointer based, except for sibling
+	calls which use incoming arguments, and thus they are deemed
 	of non-constant base in this pass.  This requires special
 	handling but also means that the frame pointer based stores
 	need not be killed upon encountering a const function call.
@@ -2516,7 +2517,7 @@ 
 		     const_call ? "const" : "memset", INSN_UID (insn));
 
 	  /* See the head comment of the frame_read field.  */
-	  if (reload_completed)
+	  if (reload_completed || SIBLING_CALL_P (insn))
 	    insn_info->frame_read = true;
 
 	  /* Loop over the active stores and remove those which are
@@ -2525,8 +2526,11 @@ 
 	    {
 	      bool remove_store = false;
 
-	      /* The stack pointer based stores are always killed.  */
-	      if (i_ptr->stack_pointer_based)
+	      /* The stack pointer based stores are always killed for
+		 non sibling calls.  Before reload, the arguments of a
+		 sibling call are frame pointer based.  */
+	      if (i_ptr->stack_pointer_based
+		  && (reload_completed || !SIBLING_CALL_P (insn)))
 	        remove_store = true;
 
 	      /* If the frame is read, the frame related stores are killed.  */
--- /dev/null	2014-11-28 19:44:51.440000000 -0500
+++ gcc/testsuite/gcc.dg/pr55023.c	2014-12-04 19:49:45.887701600 -0500
@@ -0,0 +1,33 @@ 
+/* PR rtl-optimization/55023 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-inline" } */
+
+extern void abort (void);
+typedef long long int64_t;
+
+struct foo {
+    int x;
+    int y;
+};
+
+int64_t foo(int64_t a, int64_t b, int64_t c)
+{
+    return a + b + c;
+}
+
+int64_t bar(int64_t a, struct foo bq, struct foo cq)
+{
+    int64_t b = bq.x + bq.y;
+    int64_t c = cq.x + cq.y;
+    return foo(a, b, c);
+}
+
+int main(void)
+{
+  int64_t a = 1;
+  struct foo b = { 2, 3 };
+  struct foo c = { 4, 5 };
+  if (bar (a, b, c) != 15)
+    abort ();
+  return 0;
+}