Re: simplify the argument of return_expr

Re: simplify the argument of return_expr

[Steven Bosscher]

References: http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00350.html

Hi Richard,

Your patch also re-breaks PRs 14440 and 15108, for which I should
have added test cases (my bad).

Gr.
Steven

Re: simplify the argument of return_expr

[Richard Henderson]

On Mon, Jun 07, 2004 at 03:38:45PM -0400, Jason Merrill wrote:
> My mistake, the new box I was testing on is actually x86_64.

Testing the following on amd64-linux.  It cures the problem in
the .i file provided by Jason.


r~


	* gimple-low.c (struct lower_data): Replace the_return_label and
	one_return_stmt with return_statements.
	(lower_function_body): Process the entire list of return_statements.
	(lower_return_expr): Check source value before unifying return_exprs.
	* gimplify.c (gimplify_return_expr): Force the use of a temporary
	for !aggregate_value_p.
	* tree-gimple.c: Update RETURN_EXPR grammer.

Index: gimple-low.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimple-low.c,v
retrieving revision 2.3
diff -c -p -d -r2.3 gimple-low.c
*** gimple-low.c	7 Jun 2004 17:53:02 -0000	2.3
--- gimple-low.c	7 Jun 2004 21:26:51 -0000
*************** struct lower_data
*** 47,55 ****
    /* Block the current statement belongs to.  */
    tree block;
  
!   /* Label that unifies the return statements.  */
!   tree the_return_label;
!   tree one_return_stmt;
  };
  
  static void lower_stmt (tree_stmt_iterator *, struct lower_data *);
--- 47,55 ----
    /* Block the current statement belongs to.  */
    tree block;
  
!   /* A TREE_LIST of label and return statements to be moved to the end
!      of the function.  */
!   tree return_statements;
  };
  
  static void lower_stmt (tree_stmt_iterator *, struct lower_data *);
*************** lower_function_body (void)
*** 76,83 ****
    BLOCK_CHAIN (data.block) = NULL_TREE;
    TREE_ASM_WRITTEN (data.block) = 1;
  
!   data.the_return_label = NULL_TREE;
!   data.one_return_stmt = NULL_TREE;
  
    *body_p = alloc_stmt_list ();
    i = tsi_start (*body_p);
--- 76,82 ----
    BLOCK_CHAIN (data.block) = NULL_TREE;
    TREE_ASM_WRITTEN (data.block) = 1;
  
!   data.return_statements = NULL_TREE;
  
    *body_p = alloc_stmt_list ();
    i = tsi_start (*body_p);
*************** lower_function_body (void)
*** 86,98 ****
  
    /* If we lowered any return statements, emit the representative at the
       end of the function.  */
!   if (data.one_return_stmt)
      {
!       tree t;
!       t = build (LABEL_EXPR, void_type_node, data.the_return_label);
        i = tsi_last (*body_p);
!       tsi_link_after (&i, t, TSI_CONTINUE_LINKING);
!       tsi_link_after (&i, data.one_return_stmt, TSI_CONTINUE_LINKING);
      }
  
    if (data.block != DECL_INITIAL (current_function_decl))
--- 85,107 ----
  
    /* If we lowered any return statements, emit the representative at the
       end of the function.  */
!   if (data.return_statements)
      {
!       tree t, x;
        i = tsi_last (*body_p);
! 
!       for (t = data.return_statements; t ; t = TREE_CHAIN (t))
! 	{
! 	  x = build (LABEL_EXPR, void_type_node, TREE_PURPOSE (t));
!           tsi_link_after (&i, x, TSI_CONTINUE_LINKING);
! 
! 	  /* Remove the line number from the representative return statement.
! 	     It now fills in for many such returns.  Failure to remove this
! 	     will result in incorrect results for coverage analysis.  */
! 	  x = TREE_VALUE (t);
! 	  SET_EXPR_LOCUS (x, NULL);
!           tsi_link_after (&i, x, TSI_CONTINUE_LINKING);
!         }
      }
  
    if (data.block != DECL_INITIAL (current_function_decl))
*************** lower_cond_expr (tree_stmt_iterator *tsi
*** 392,407 ****
  static void
  lower_return_expr (tree_stmt_iterator *tsi, struct lower_data *data)
  {
!   tree stmt, label = data->the_return_label;
  
!   if (!label)
      {
!       data->the_return_label = label = create_artificial_label ();
!       data->one_return_stmt = tsi_stmt (*tsi);
      }
  
!   stmt = build (GOTO_EXPR, void_type_node, label);
!   tsi_link_before (tsi, stmt, TSI_SAME_STMT);
    tsi_delink (tsi);
  }
  \f
--- 401,437 ----
  static void
  lower_return_expr (tree_stmt_iterator *tsi, struct lower_data *data)
  {
!   tree stmt = tsi_stmt (*tsi);
!   tree value, t, label;
  
!   /* Extract the value being returned.  */
!   value = TREE_OPERAND (stmt, 0);
!   if (value && TREE_CODE (value) == MODIFY_EXPR)
!     value = TREE_OPERAND (value, 1);
! 
!   /* Match this up with an existing return statement that's been created.  */
!   for (t = data->return_statements; t ; t = TREE_CHAIN (t))
      {
!       tree tvalue = TREE_OPERAND (TREE_VALUE (t), 0);
!       if (tvalue && TREE_CODE (tvalue) == MODIFY_EXPR)
! 	tvalue = TREE_OPERAND (tvalue, 1);
! 
!       if (value == tvalue)
! 	{
! 	  label = TREE_PURPOSE (t);
! 	  goto found;
! 	}
      }
  
!   /* Not found.  Create a new label and record the return statement.  */
!   label = create_artificial_label ();
!   data->return_statements = tree_cons (label, stmt, data->return_statements);
! 
!   /* Generate a goto statement and remove the return statement.  */
!  found:
!   t = build (GOTO_EXPR, void_type_node, label);
!   SET_EXPR_LOCUS (t, EXPR_LOCUS (stmt));
!   tsi_link_before (tsi, t, TSI_SAME_STMT);
    tsi_delink (tsi);
  }
  \f
Index: gimplify.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimplify.c,v
retrieving revision 2.11
diff -c -p -d -r2.11 gimplify.c
*** gimplify.c	7 Jun 2004 17:53:02 -0000	2.11
--- gimplify.c	7 Jun 2004 21:26:51 -0000
*************** static struct gimplify_ctx
*** 54,59 ****
--- 54,60 ----
    tree conditional_cleanups;
    int conditions;
    tree exit_label;
+   tree return_temp;
    varray_type case_labels;
    /* The formal temporary table.  Should this be persistent?  */
    htab_t temp_htab;
*************** static enum gimplify_status
*** 876,882 ****
  gimplify_return_expr (tree stmt, tree *pre_p)
  {
    tree ret_expr = TREE_OPERAND (stmt, 0);
!   tree result;
  
    if (!ret_expr || TREE_CODE (ret_expr) == RESULT_DECL)
      return GS_ALL_DONE;
--- 877,883 ----
  gimplify_return_expr (tree stmt, tree *pre_p)
  {
    tree ret_expr = TREE_OPERAND (stmt, 0);
!   tree result_decl, result;
  
    if (!ret_expr || TREE_CODE (ret_expr) == RESULT_DECL)
      return GS_ALL_DONE;
*************** gimplify_return_expr (tree stmt, tree *p
*** 885,908 ****
      return GS_ERROR;
  
    if (VOID_TYPE_P (TREE_TYPE (TREE_TYPE (current_function_decl))))
!     result = NULL_TREE;
    else
      {
!       result = TREE_OPERAND (ret_expr, 0);
  #ifdef ENABLE_CHECKING
        if ((TREE_CODE (ret_expr) != MODIFY_EXPR
  	   && TREE_CODE (ret_expr) != INIT_EXPR)
! 	  || TREE_CODE (result) != RESULT_DECL)
  	abort ();
  #endif
      }
  
!   /* We need to pass the full MODIFY_EXPR down so that special handling
!      can replace it with something else.  */
    gimplify_stmt (&TREE_OPERAND (stmt, 0));
    append_to_statement_list (TREE_OPERAND (stmt, 0), pre_p);
  
!   TREE_OPERAND (stmt, 0) = result;
  
    return GS_ALL_DONE;
  }
--- 886,936 ----
      return GS_ERROR;
  
    if (VOID_TYPE_P (TREE_TYPE (TREE_TYPE (current_function_decl))))
!     result_decl = NULL_TREE;
    else
      {
!       result_decl = TREE_OPERAND (ret_expr, 0);
  #ifdef ENABLE_CHECKING
        if ((TREE_CODE (ret_expr) != MODIFY_EXPR
  	   && TREE_CODE (ret_expr) != INIT_EXPR)
! 	  || TREE_CODE (result_decl) != RESULT_DECL)
  	abort ();
  #endif
      }
  
!   /* If aggregate_value_p is true, then we can return the bare RESULT_DECL.
!      Recall that aggregate_value_p is FALSE for any aggregate type that is
!      returned in registers.  If we're returning values in registers, then
!      we don't want to extend the lifetime of the RESULT_DECL, particularly
!      across another call.  In addition, for those aggregates for which 
!      hard_function_value generates a PARALLEL, we'll abort during normal
!      expansion of structure assignments; there's special code in expand_return
!      to handle this case that does not exist in expand_expr.  */
!   if (!result_decl
!       || aggregate_value_p (result_decl, TREE_TYPE (current_function_decl)))
!     result = result_decl;
!   else if (gimplify_ctxp->return_temp)
!     result = gimplify_ctxp->return_temp;
!   else
!     {
!       result = create_tmp_var (TREE_TYPE (result_decl), NULL);
!       gimplify_ctxp->return_temp = result;
!     }
! 
!   /* Smash the lhs of the MODIFY_EXPR to the temporary we plan to use.
!      Then gimplify the whole thing.  */
!   if (result != result_decl)
!     TREE_OPERAND (ret_expr, 0) = result;
    gimplify_stmt (&TREE_OPERAND (stmt, 0));
    append_to_statement_list (TREE_OPERAND (stmt, 0), pre_p);
  
!   /* If we didn't use a temporary, then the result is just the result_decl.
!      Otherwise we need a simple copy.  This should already be gimple.  */
!   if (result == result_decl)
!     ret_expr = result;
!   else
!     ret_expr = build (MODIFY_EXPR, TREE_TYPE (result), result_decl, result);
!   TREE_OPERAND (stmt, 0) = ret_expr;
  
    return GS_ALL_DONE;
  }
Index: tree-gimple.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-gimple.c,v
retrieving revision 2.4
diff -c -p -d -r2.4 tree-gimple.c
*** tree-gimple.c	7 Jun 2004 17:53:02 -0000	2.4
--- tree-gimple.c	7 Jun 2004 21:26:51 -0000
*************** Boston, MA 02111-1307, USA.  */
*** 79,85 ****
         GOTO_EXPR
           op0 -> LABEL_DECL | '*' ID
       | RETURN_EXPR
!          op0 -> RESULT_DECL | NULL_TREE
       | THROW_EXPR?  do we need/want such a thing for opts, perhaps
           to generate an ERT_THROW region?  I think so.
  	 Hmm...this would only work at the GIMPLE level, where we know that
--- 79,87 ----
         GOTO_EXPR
           op0 -> LABEL_DECL | '*' ID
       | RETURN_EXPR
!          op0 -> NULL_TREE
! 	      | RESULT_DECL
! 	      | MODIFY_EXPR -> RESULT_DECL, varname
       | THROW_EXPR?  do we need/want such a thing for opts, perhaps
           to generate an ERT_THROW region?  I think so.
  	 Hmm...this would only work at the GIMPLE level, where we know that

Re: simplify the argument of return_expr

[Jason Merrill]

On Mon, 7 Jun 2004 12:31:27 -0700, Richard Henderson <rth@twiddle.net> wrote:

> On Mon, Jun 07, 2004 at 03:12:01PM -0400, Jason Merrill wrote:
>> This patch breaks bootstrap for me; I get an ICE in change_address_1
>> building genrecog.
>
> Really?
>
>> What platform did you test this on?  I'm seeing the bootstrap failure on
>> x86 linux.
>
> I tested on x86-linux.

My mistake, the new box I was testing on is actually x86_64.

Jason

Re: simplify the argument of return_expr

[Richard Henderson]

On Mon, Jun 07, 2004 at 03:12:01PM -0400, Jason Merrill wrote:
> This patch breaks bootstrap for me; I get an ICE in change_address_1
> building genrecog.

Really?

> What platform did you test this on?  I'm seeing the bootstrap failure on
> x86 linux.

I tested on x86-linux.


r~

Re: simplify the argument of return_expr

[Jason Merrill]

This patch breaks bootstrap for me; I get an ICE in change_address_1
building genrecog.

I'm afraid you're running into the problem hinted at by

-	 (maybe -> RESULT_DECL | NULL_TREE? seems like some of expand_return
-	  depends on getting a MODIFY_EXPR.)

There's a lot of magic in expand_return for dealing with returning
aggregates in multiple registers that should really move to
expand_assignment or some such.  That needs to happen before your patch
goes in.

What platform did you test this on?  I'm seeing the bootstrap failure on
x86 linux.

Jason

Re: simplify the argument of return_expr

[Steven Bosscher]

On Monday 07 June 2004 20:04, Richard Henderson wrote:
> I'm not quite sure what to do about this.  Perhaps the simplest
> solution is to add a line number field to the edge structure.
> Thoughts about this?

We're seeing the same thing on the tree profiling branch,
and Honza had some ideas on how to fix it.  I couldn't
find it at once, but here is an URL:
http://gcc.gnu.org/ml/gcc-patches/2004-05/msg01807.html
with a bug fix follow-up:
http://gcc.gnu.org/ml/gcc-patches/2004-05/msg01811.html

Just another reason to get the cfgexpand stuff merged soon ;-)

Gr.
Steven

simplify the argument of return_expr

[Richard Henderson]

The effect of this is to change the argument of the return_expr
to only null or a result_decl.  Further, that there should be
exactly one return_expr in the function, and that it should be
at the end of the function, outside other constructs.

All by itself this should have little impact on generated code,
since the tree-rtl expansion phase does exactly the same thing
already.  However, it does have two effects at the tree level:

First, it exposes the assignment to <retval> to the tree-level
if conversion routines.

Second, it will simplify the various tree-level optimizers,
in that we no longer have to consider complex expressions inside
the return_expr.  For instance, the return_expr handling in
tree-eh.c is now dead.  I believe there are other similar cases
but havn't yet looked.

There are two regressions with this change.  Both the C and the
C++ gcov tests have new failures.  The problem here is an 
continuation of the existing problem that (1) we remove all goto
statements and (2) we don't record what line number they may
have been associated with.

I'm not quite sure what to do about this.  Perhaps the simplest
solution is to add a line number field to the edge structure.
Thoughts about this?


r~


        * gimple-low.c (struct lower_data): Add the_return_label and
        one_return_stmt.
        (lower_function_body): Initialize and use them.
        (lower_return_expr): New.
        (lower_stmt): Call it.
        * gimplify.c (gimplify_return_expr): Force the argument to be either
        null or a result_decl.
        * tree-gimple.c: Update gimple grammer to match.
        * tree-ssa-copyrename.c (copy_rename_partition_coalesce): Deny
        coalescing of result_decls.
testsuite/
        * gcc.dg/tree-ssa/20030728-1.c: Fixup return value to not match
        if temporaries.

Index: gcc/gimple-low.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimple-low.c,v
retrieving revision 2.2
diff -u -p -r2.2 gimple-low.c
--- gcc/gimple-low.c	14 May 2004 02:29:20 -0000	2.2
+++ gcc/gimple-low.c	7 Jun 2004 17:46:31 -0000
@@ -46,11 +46,16 @@ struct lower_data
 {
   /* Block the current statement belongs to.  */
   tree block;
+
+  /* Label that unifies the return statements.  */
+  tree the_return_label;
+  tree one_return_stmt;
 };
 
 static void lower_stmt (tree_stmt_iterator *, struct lower_data *);
 static void lower_bind_expr (tree_stmt_iterator *, struct lower_data *);
 static void lower_cond_expr (tree_stmt_iterator *, struct lower_data *);
+static void lower_return_expr (tree_stmt_iterator *, struct lower_data *);
 static bool expand_var_p (tree);
 
 /* Lowers the body of current_function_decl.  */
@@ -71,11 +76,25 @@ lower_function_body (void)
   BLOCK_CHAIN (data.block) = NULL_TREE;
   TREE_ASM_WRITTEN (data.block) = 1;
 
+  data.the_return_label = NULL_TREE;
+  data.one_return_stmt = NULL_TREE;
+
   *body_p = alloc_stmt_list ();
   i = tsi_start (*body_p);
   tsi_link_after (&i, bind, TSI_NEW_STMT);
   lower_bind_expr (&i, &data);
 
+  /* If we lowered any return statements, emit the representative at the
+     end of the function.  */
+  if (data.one_return_stmt)
+    {
+      tree t;
+      t = build (LABEL_EXPR, void_type_node, data.the_return_label);
+      i = tsi_last (*body_p);
+      tsi_link_after (&i, t, TSI_CONTINUE_LINKING);
+      tsi_link_after (&i, data.one_return_stmt, TSI_CONTINUE_LINKING);
+    }
+
   if (data.block != DECL_INITIAL (current_function_decl))
     abort ();
   BLOCK_SUBBLOCKS (data.block)
@@ -136,6 +155,9 @@ lower_stmt (tree_stmt_iterator *tsi, str
     case COND_EXPR:
       lower_cond_expr (tsi, data);
       return;
+    case RETURN_EXPR:
+      lower_return_expr (tsi, data);
+      return;
 
     case TRY_FINALLY_EXPR:
     case TRY_CATCH_EXPR:
@@ -151,7 +173,6 @@ lower_stmt (tree_stmt_iterator *tsi, str
       
     case NOP_EXPR:
     case ASM_EXPR:
-    case RETURN_EXPR:
     case MODIFY_EXPR:
     case CALL_EXPR:
     case GOTO_EXPR:
@@ -367,6 +388,22 @@ lower_cond_expr (tree_stmt_iterator *tsi
 
   tsi_next (tsi);
 }
+
+static void
+lower_return_expr (tree_stmt_iterator *tsi, struct lower_data *data)
+{
+  tree stmt, label = data->the_return_label;
+
+  if (!label)
+    {
+      data->the_return_label = label = create_artificial_label ();
+      data->one_return_stmt = tsi_stmt (*tsi);
+    }
+
+  stmt = build (GOTO_EXPR, void_type_node, label);
+  tsi_link_before (tsi, stmt, TSI_SAME_STMT);
+  tsi_delink (tsi);
+}
 \f
 
 /* Record the variables in VARS.  */
@@ -468,5 +505,3 @@ struct tree_opt_pass pass_remove_useless
   0,					/* todo_flags_start */
   TODO_dump_func			/* todo_flags_finish */
 };
-
-
Index: gcc/gimplify.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gimplify.c,v
retrieving revision 2.10
diff -u -p -r2.10 gimplify.c
--- gcc/gimplify.c	30 May 2004 18:32:26 -0000	2.10
+++ gcc/gimplify.c	7 Jun 2004 17:46:32 -0000
@@ -899,54 +899,11 @@ gimplify_return_expr (tree stmt, tree *p
 
   /* We need to pass the full MODIFY_EXPR down so that special handling
      can replace it with something else.  */
-  gimplify_stmt (&ret_expr);
+  gimplify_stmt (&TREE_OPERAND (stmt, 0));
+  append_to_statement_list (TREE_OPERAND (stmt, 0), pre_p);
 
-  if (result == NULL_TREE)
-    TREE_OPERAND (stmt, 0) = NULL_TREE;
-  else if (ret_expr == TREE_OPERAND (stmt, 0))
-    /* It was already GIMPLE.  */
-    return GS_ALL_DONE;
-  else
-    {
-      /* If there's still a MODIFY_EXPR of the RESULT_DECL after
-	 gimplification, find it so we can put it in the RETURN_EXPR.  */
-      tree ret = NULL_TREE;
-
-      if (TREE_CODE (ret_expr) == STATEMENT_LIST)
-	{
-	  tree_stmt_iterator si;
-	  for (si = tsi_start (ret_expr); !tsi_end_p (si); tsi_next (&si))
-	    {
-	      tree sub = tsi_stmt (si);
-	      if (TREE_CODE (sub) == MODIFY_EXPR
-		  && TREE_OPERAND (sub, 0) == result)
-		{
-		  ret = sub;
-		  if (tsi_one_before_end_p (si))
-		    tsi_delink (&si);
-		  else
-		    {
-		      /* If there were posteffects after the MODIFY_EXPR,
-			 we need a temporary.  */
-		      tree tmp = create_tmp_var (TREE_TYPE (result), "retval");
-		      TREE_OPERAND (ret, 0) = tmp;
-		      ret = build (MODIFY_EXPR, TREE_TYPE (result),
-				   result, tmp);
-		    }
-		  break;
-		}
-	    }
-	}
-
-      if (ret)
-	TREE_OPERAND (stmt, 0) = ret;
-      else
-	/* The return value must be set up some other way.  Just tell
-	   expand_return that we're returning the RESULT_DECL.  */
-	TREE_OPERAND (stmt, 0) = result;
-    }
+  TREE_OPERAND (stmt, 0) = result;
 
-  append_to_statement_list (ret_expr, pre_p);
   return GS_ALL_DONE;
 }
 
Index: gcc/tree-gimple.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-gimple.c,v
retrieving revision 2.3
diff -u -p -r2.3 tree-gimple.c
--- gcc/tree-gimple.c	26 May 2004 22:36:49 -0000	2.3
+++ gcc/tree-gimple.c	7 Jun 2004 17:46:32 -0000
@@ -79,9 +79,7 @@ Boston, MA 02111-1307, USA.  */
        GOTO_EXPR
          op0 -> LABEL_DECL | '*' ID
      | RETURN_EXPR
-         op0 -> modify-stmt | NULL_TREE
-	 (maybe -> RESULT_DECL | NULL_TREE? seems like some of expand_return
-	  depends on getting a MODIFY_EXPR.)
+         op0 -> RESULT_DECL | NULL_TREE
      | THROW_EXPR?  do we need/want such a thing for opts, perhaps
          to generate an ERT_THROW region?  I think so.
 	 Hmm...this would only work at the GIMPLE level, where we know that
Index: gcc/tree-ssa-copyrename.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-ssa-copyrename.c,v
retrieving revision 2.2
diff -u -p -r2.2 tree-ssa-copyrename.c
--- gcc/tree-ssa-copyrename.c	14 May 2004 02:29:23 -0000	2.2
+++ gcc/tree-ssa-copyrename.c	7 Jun 2004 17:46:32 -0000
@@ -187,6 +187,13 @@ copy_rename_partition_coalesce (var_map 
       return;
     }
 
+  if ((TREE_CODE (root1) == RESULT_DECL) != (TREE_CODE (root2) == RESULT_DECL))
+    {
+      if (debug)
+        fprintf (debug, " : One root a RESULT_DECL. No coalesce.\n");
+      return;
+    }
+
   gimp1 = is_gimple_tmp_var (root1);
   gimp2 = is_gimple_tmp_var (root2);
 
Index: gcc/testsuite/gcc.dg/tree-ssa/20030728-1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/testsuite/gcc.dg/tree-ssa/20030728-1.c,v
retrieving revision 1.2
diff -u -p -r1.2 20030728-1.c
--- gcc/testsuite/gcc.dg/tree-ssa/20030728-1.c	13 May 2004 06:40:51 -0000	1.2
+++ gcc/testsuite/gcc.dg/tree-ssa/20030728-1.c	7 Jun 2004 17:46:32 -0000
@@ -35,7 +35,7 @@ objects_must_conflict_p (t1, t2)
 
   if ((t1->common.code == ARRAY_TYPE) != (t2
                                           && t2->common.code == ARRAY_TYPE))
-    return 0;
+    return 11;
 
 
   return foo (t2 ? get_alias_set (t2) : 0);