Simple returns are broken in gcc 3.X

Simple returns are broken in gcc 3.X

[John David Anglin]

In looking into why g++ didn't work for vax-dec-ultrix4.3, I have found
that simple returns are broken.  Compiling this small test program

static char *
srealloc (char *p, char *q, int i)
{
  if (i == 0)
    return p;
  else
    return q;
}

yields

#NO_APP
	.text
	.align 1
__Z8sreallocPcS_i:
	.word 0x0
	subl2 $12,sp
	tstl 12(ap)
	jneq L2
	ret
L2:
	ret

As can been seen, the return values are not loaded into the return register
r0.

The problem is that expand_value_return () doesn't handle properly return
values in pseudos (see treatment in expand_function_end ()).  Further,
comparing what is done in expand_function_end and expand_null_return_1,
there appears to be other problems.  For example, instrumentation doesn't
work.  There could be problems with functtions that call alloca, etc.

It looks to me like there needs to be a common set of code used to expand
returns.  Thoughts?

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: Simple returns are broken in gcc 3.X

[Richard Henderson]

On Mon, Jul 30, 2001 at 06:33:14PM -0400, John David Anglin wrote:
> It looks to me like there needs to be a common set of code used to expand
> returns.  Thoughts?

You'd do just as well to disable the "return" pattern until
after reload.  Given that most other targets can't guarantee
to use return directly until then you'll save yourself from
executing code paths that no one else uses.

Further, the post-reload optimizers can easily put the return
everywhere it's needed, so you don't actually gain anything
by generating it early.


r~

Re: Simple returns are broken in gcc 3.X

[John David Anglin]

> Further, the post-reload optimizers can easily put the return
> everywhere it's needed, so you don't actually gain anything
> by generating it early.

Wouldn't it be better then to never output a simple return in
expand_null_return_1?  Something like: 

--- stmt.c.orig	Thu May 17 07:37:30 2001
+++ stmt.c	Mon Jul 30 23:27:50 2001
@@ -403,7 +403,7 @@
 static void expand_nl_goto_receivers	PARAMS ((struct nesting *));
 static void fixup_gotos			PARAMS ((struct nesting *, rtx, tree,
 					       rtx, int));
-static void expand_null_return_1	PARAMS ((rtx, int));
+static void expand_null_return_1	PARAMS ((rtx));
 static void expand_value_return		PARAMS ((rtx));
 static int tail_recursion_args		PARAMS ((tree, tree));
 static void expand_cleanups		PARAMS ((tree, tree, int, int));
@@ -2805,7 +2805,6 @@
 void
 expand_null_return ()
 {
-  struct nesting *block = block_stack;
   rtx last_insn = get_last_insn ();
 
   /* If this function was declared to return a value, but we
@@ -2813,13 +2812,7 @@
      propogated live to the rest of the function.  */
   clobber_return_register ();
 
-  /* Does any pending block have cleanups?  */
-  while (block && block->data.block.cleanups == 0)
-    block = block->next;
-
-  /* If yes, use a goto to return, since that runs cleanups.  */
-
-  expand_null_return_1 (last_insn, block != 0);
+  expand_null_return_1 (last_insn);
 }
 
 /* Generate RTL to return from the current function, with value VAL.  */
@@ -2828,7 +2821,6 @@
 expand_value_return (val)
      rtx val;
 {
-  struct nesting *block = block_stack;
   rtx last_insn = get_last_insn ();
   rtx return_reg = DECL_RTL (DECL_RESULT (current_function_decl));
 
@@ -2855,27 +2847,15 @@
 	emit_move_insn (return_reg, val);
     }
 
-  /* Does any pending block have cleanups?  */
-
-  while (block && block->data.block.cleanups == 0)
-    block = block->next;
-
-  /* If yes, use a goto to return, since that runs cleanups.
-     Use LAST_INSN to put cleanups *before* the move insn emitted above.  */
-
-  expand_null_return_1 (last_insn, block != 0);
+  expand_null_return_1 (last_insn);
 }
 
 /* Output a return with no value.  If LAST_INSN is nonzero,
-   pretend that the return takes place after LAST_INSN.
-   If USE_GOTO is nonzero then don't use a return instruction;
-   go to the return label instead.  This causes any cleanups
-   of pending blocks to be executed normally.  */
+   pretend that the return takes place after LAST_INSN.  */
 
 static void
-expand_null_return_1 (last_insn, use_goto)
+expand_null_return_1 (last_insn)
      rtx last_insn;
-     int use_goto;
 {
   rtx end_label = cleanup_label ? cleanup_label : return_label;
 
@@ -2883,27 +2863,8 @@
   do_pending_stack_adjust ();
   last_expr_type = 0;
 
-  /* PCC-struct return always uses an epilogue.  */
-  if (current_function_returns_pcc_struct || use_goto)
-    {
-      if (end_label == 0)
-	end_label = return_label = gen_label_rtx ();
-      expand_goto_internal (NULL_TREE, end_label, last_insn);
-      return;
-    }
-
-  /* Otherwise output a simple return-insn if one is available,
-     unless it won't do the job.  */
-#ifdef HAVE_return
-  if (HAVE_return && use_goto == 0 && cleanup_label == 0)
-    {
-      emit_jump_insn (gen_return ());
-      emit_barrier ();
-      return;
-    }
-#endif
-
-  /* Otherwise jump to the epilogue.  */
+  if (end_label == 0)
+     end_label = return_label = gen_label_rtx ();
   expand_goto_internal (NULL_TREE, end_label, last_insn);
 }
 \f

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: Simple returns are broken in gcc 3.X

[Richard Henderson]

On Mon, Jul 30, 2001 at 11:35:45PM -0400, John David Anglin wrote:
> Wouldn't it be better then to never output a simple return in
> expand_null_return_1?

Err, yes it would.  There's another bit in expand_function_end.


r~

Re: Simple returns are broken in gcc 3.X

[John David Anglin]

> Err, yes it would.  There's another bit in expand_function_end.

Isn't that bit necessary if there isn't an epilogue?

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: Simple returns are broken in gcc 3.X

[Richard Henderson]

On Tue, Jul 31, 2001 at 10:39:58AM -0400, John David Anglin wrote:
> > Err, yes it would.  There's another bit in expand_function_end.
> 
> Isn't that bit necessary if there isn't an epilogue?

No, look at thread_prologue_and_epilogue_insns.


r~

Re: Simple returns are broken in gcc 3.X

[John David Anglin]

> On Tue, Jul 31, 2001 at 10:39:58AM -0400, John David Anglin wrote:
> > > Err, yes it would.  There's another bit in expand_function_end.
> > 
> > Isn't that bit necessary if there isn't an epilogue?
> 
> No, look at thread_prologue_and_epilogue_insns.

It looks like the "optimize" test needs to be removed from
thread_prologue_and_epilogue_insns so that emit_return_into_block
is always called.  Is that OK?  I presume that this is there
because we didn't have a proper CFG when not optimizing.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: Simple returns are broken in gcc 3.X

[Richard Henderson]

On Tue, Jul 31, 2001 at 01:36:41PM -0400, John David Anglin wrote:
> It looks like the "optimize" test needs to be removed from
> thread_prologue_and_epilogue_insns so that emit_return_into_block
> is always called.  Is that OK?  I presume that this is there
> because we didn't have a proper CFG when not optimizing.

Hmm.  No, I suspect having a single return point -- and in particular no
conditional returns -- is going to make debuggers happier with features
like "xbreak".

Grepping through sources, vax is the only target that does not define
either HAVE_epilogue or TARGET_ASM_FUNCTION_EPILOGUE.

I suggest that you add

(define_expand "epilogue"
  [(return)]
  ""
  "")


r~

Re: Simple returns are broken in gcc 3.X

[John David Anglin]

> I suggest that you add
> 
> (define_expand "epilogue"
>   [(return)]
>   ""
>   "")

It may not be necessary.  I am trying to rework
thread_prologue_and_epilogue_insns to always output a return
in the HAVE_return case.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: Simple returns are broken in gcc 3.X

[Richard Henderson]

On Tue, Jul 31, 2001 at 04:43:15PM -0400, John David Anglin wrote:
> It may not be necessary.  I am trying to rework
> thread_prologue_and_epilogue_insns to always output a return
> in the HAVE_return case.

Hum.  We'll see.  I still think you'd be better served adding the
epilogue pattern.  It's clearer to categorize HAVE_return as the
optimization and HAVE_epilogue as the requirement.


r~

Re: Simple returns are broken in gcc 3.X

[John David Anglin]

> On Tue, Jul 31, 2001 at 04:43:15PM -0400, John David Anglin wrote:
> > It may not be necessary.  I am trying to rework
> > thread_prologue_and_epilogue_insns to always output a return
> > in the HAVE_return case.
> 
> Hum.  We'll see.  I still think you'd be better served adding the
> epilogue pattern.  It's clearer to categorize HAVE_return as the
> optimization and HAVE_epilogue as the requirement.

I can't disagree with your logic.

In looking at thread_prologue_and_epilogue_insns, I am having
difficulty understanding how "returns" are generated when an exit
block has no non-fake predecessors or no fall-thru predecessor.
In these two situations, the code jumps to epilogue_done and the
epilogue isn't expanded. 

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

Re: Simple returns are broken in gcc 3.X

[Richard Henderson]

On Tue, Jul 31, 2001 at 07:06:25PM -0400, John David Anglin wrote:
> In looking at thread_prologue_and_epilogue_insns, I am having
> difficulty understanding how "returns" are generated when an exit
> block has no non-fake predecessors or no fall-thru predecessor.

They aren't generated in this case because that means that the
function doesn't return (normally) at all.  Perhaps that bit is
deserving of a comment...

There are two other kinds of edges that can come to EXIT_BLOCK:

  (1) If EDGE_ABNORMAL_CALL, then we exit the function by a sibling call.
  (2) If !EDGE_FALLTHRU, then we exit via a return insn.  Given that
      we're removing HAVE_return generation from early in the compiler,
      this should no longer happen at this point.


r~

Re: Simple returns are broken in gcc 3.X

[Richard Henderson]

On Thu, Aug 09, 2001 at 06:12:04PM -0400, John David Anglin wrote:
> 	* stmt.c (expand_null_return_1): Remove code to generate simple returns 
> 	and "use_goto" argument.
> 	(expand_null_return, expand_value_return): Update all callers.
> 	* function.c (expand_function_end): Remove code to generate simple
> 	return.
> 	* config/vax/vax.md (epilogue): New expander for function return.
> 	* doc/md.texi (epilogue): Remove "if defined".

Ok.


r~

Re: Simple returns are broken in gcc 3.X

[John David Anglin]

> In looking into why g++ didn't work for vax-dec-ultrix4.3, I have found
> that simple returns are broken.  Compiling this small test program
> 
> static char *
> srealloc (char *p, char *q, int i)
> {
>   if (i == 0)
>     return p;
>   else
>     return q;
> }
> 
> yields
> 
> #NO_APP
> 	.text
> 	.align 1
> __Z8sreallocPcS_i:
> 	.word 0x0
> 	subl2 $12,sp
> 	tstl 12(ap)
> 	jneq L2
> 	ret
> L2:
> 	ret

This patch fixes the above problem by always forcing a "goto" return.
The patch only affects the vax since it is the only remaining port that
doesn't define an epilogue and allows the expansion of simple returns
prior to reload.  This patch changes the vax port to use the same
code patch for return generation as all others.  As a side benefit, the
change also fixes function instrumentation on the vax.

The above behavior is a regression versus previous gcc versions since
g++ was functional on the vax in version 2.8.1.

Full bootstrap has been done with vax-dec-ultrix4.3 with 3.0 branch.
Bootstrap and regression checked with 3.0 branch on hppa1.1-hp-hpux10.20
and i686-pc-linux-gnu.  Bootstrap and regression checked with 3.1
mainline on i686-pc-linux-gnu.

OK for main?  I would like it on the branch as well but there are a
couple of other patches needed before C++ will be fully functional
on the vax.  Thus, it can wait until after 3.0.1 is released.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

2001-08-09  John David Anglin  <dave@hiauly1.hia.nrc.ca>

	* stmt.c (expand_null_return_1): Remove code to generate simple returns 
	and "use_goto" argument.
	(expand_null_return, expand_value_return): Update all callers.
	* function.c (expand_function_end): Remove code to generate simple
	return.
	* config/vax/vax.md (epilogue): New expander for function return.
	* doc/md.texi (epilogue): Remove "if defined".

--- stmt.c.orig	Tue Jul 31 10:22:56 2001
+++ stmt.c	Wed Aug  1 15:52:05 2001
@@ -403,7 +403,7 @@
 static void expand_nl_goto_receivers	PARAMS ((struct nesting *));
 static void fixup_gotos			PARAMS ((struct nesting *, rtx, tree,
 					       rtx, int));
-static void expand_null_return_1	PARAMS ((rtx, int));
+static void expand_null_return_1	PARAMS ((rtx));
 static void expand_value_return		PARAMS ((rtx));
 static int tail_recursion_args		PARAMS ((tree, tree));
 static void expand_cleanups		PARAMS ((tree, tree, int, int));
@@ -2863,7 +2863,6 @@
 void
 expand_null_return ()
 {
-  struct nesting *block = block_stack;
   rtx last_insn = get_last_insn ();
 
   /* If this function was declared to return a value, but we
@@ -2871,13 +2870,7 @@
      propogated live to the rest of the function.  */
   clobber_return_register ();
 
-  /* Does any pending block have cleanups?  */
-  while (block && block->data.block.cleanups == 0)
-    block = block->next;
-
-  /* If yes, use a goto to return, since that runs cleanups.  */
-
-  expand_null_return_1 (last_insn, block != 0);
+  expand_null_return_1 (last_insn);
 }
 
 /* Generate RTL to return from the current function, with value VAL.  */
@@ -2886,7 +2879,6 @@
 expand_value_return (val)
      rtx val;
 {
-  struct nesting *block = block_stack;
   rtx last_insn = get_last_insn ();
   rtx return_reg = DECL_RTL (DECL_RESULT (current_function_decl));
 
@@ -2913,27 +2905,15 @@
 	emit_move_insn (return_reg, val);
     }
 
-  /* Does any pending block have cleanups?  */
-
-  while (block && block->data.block.cleanups == 0)
-    block = block->next;
-
-  /* If yes, use a goto to return, since that runs cleanups.
-     Use LAST_INSN to put cleanups *before* the move insn emitted above.  */
-
-  expand_null_return_1 (last_insn, block != 0);
+  expand_null_return_1 (last_insn);
 }
 
 /* Output a return with no value.  If LAST_INSN is nonzero,
-   pretend that the return takes place after LAST_INSN.
-   If USE_GOTO is nonzero then don't use a return instruction;
-   go to the return label instead.  This causes any cleanups
-   of pending blocks to be executed normally.  */
+   pretend that the return takes place after LAST_INSN.  */
 
 static void
-expand_null_return_1 (last_insn, use_goto)
+expand_null_return_1 (last_insn)
      rtx last_insn;
-     int use_goto;
 {
   rtx end_label = cleanup_label ? cleanup_label : return_label;
 
@@ -2941,27 +2921,8 @@
   do_pending_stack_adjust ();
   last_expr_type = 0;
 
-  /* PCC-struct return always uses an epilogue.  */
-  if (current_function_returns_pcc_struct || use_goto)
-    {
-      if (end_label == 0)
-	end_label = return_label = gen_label_rtx ();
-      expand_goto_internal (NULL_TREE, end_label, last_insn);
-      return;
-    }
-
-  /* Otherwise output a simple return-insn if one is available,
-     unless it won't do the job.  */
-#ifdef HAVE_return
-  if (HAVE_return && use_goto == 0 && cleanup_label == 0)
-    {
-      emit_jump_insn (gen_return ());
-      emit_barrier ();
-      return;
-    }
-#endif
-
-  /* Otherwise jump to the epilogue.  */
+  if (end_label == 0)
+     end_label = return_label = gen_label_rtx ();
   expand_goto_internal (NULL_TREE, end_label, last_insn);
 }
 \f
--- function.c.orig	Sat Jun  9 15:22:26 2001
+++ function.c	Wed Aug  1 12:40:23 2001
@@ -6949,18 +6949,6 @@
      instead of using the general framework.  */
   use_return_register ();
 
-  /* Output a return insn if we are using one.
-     Otherwise, let the rtl chain end here, to drop through
-     into the epilogue.  */
-
-#ifdef HAVE_return
-  if (HAVE_return)
-    {
-      emit_jump_insn (gen_return ());
-      emit_barrier ();
-    }
-#endif
-
   /* Fix up any gotos that jumped out to the outermost
      binding level of the function.
      Must follow emitting RETURN_LABEL.  */
--- config/vax/vax.md.orig	Sun Jan 14 04:08:51 2001
+++ config/vax/vax.md	Tue Jul 31 22:08:45 2001
@@ -1926,6 +1926,15 @@
   ""
   "ret")
 
+(define_expand "epilogue"
+  [(return)]
+  ""
+  "
+{
+  emit_jump_insn (gen_return ());
+  DONE;
+}")
+
 (define_insn "nop"
   [(const_int 0)]
   ""
--- doc/md.texi.orig	Thu Jun 14 18:54:21 2001
+++ doc/md.texi	Thu Aug  9 12:24:04 2001
@@ -2859,7 +2859,7 @@
 
 @cindex @code{epilogue} instruction pattern
 @item @samp{epilogue}
-This pattern, if defined, emits RTL for exit from a function.  The function
+This pattern emits RTL for exit from a function.  The function
 exit is responsible for deallocating the stack frame, restoring callee saved
 registers and emitting the return instruction.