Patchwork Release virtual SSA names properly

login
register
mail settings
Submitter Richard Guenther
Date Feb. 10, 2012, 1:55 p.m.
Message ID <alpine.LNX.2.00.1202101450090.4999@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/140650/
State New
Headers show

Comments

Richard Guenther - Feb. 10, 2012, 1:55 p.m.
I've played with
Richard Guenther - Feb. 10, 2012, 3:34 p.m.
On Fri, 10 Feb 2012, Richard Guenther wrote:

> 
> I've played with
> 
> Index: tree-ssa.c
> ===================================================================
> --- tree-ssa.c  (revision 184088)
> +++ tree-ssa.c  (working copy)
> @@ -944,6 +944,11 @@ verify_ssa (bool check_modified_stmt)
>           if (!gimple_nop_p (stmt))
>             {
>               basic_block bb = gimple_bb (stmt);
> +             if (!bb)
> +               {
> +                 error ("Unreleased SSA name");
> +                 goto err;
> +               }
>               verify_def (bb, definition_block,
>                           name, stmt, !is_gimple_reg (name));
>  
> and noticed we do not properly free a lot of virtual SSA names, leaking
> both SSA names, stmts and operands they point to over time.
> 
> The following patch cures the cases I hit with running the tree-ssa.exp
> testsuite.  More issues exist, and I guess we should make a more
> verbose variant of the above the default in 4.8 to catch these issues
> (there is a slight issue with this when verify_ssa is invoked during
> loop transforms and the vectorizer has pattern stmts built that are
> not in a basic block ... so maybe we should put the above into some
> other verifier).
> 
> Bootstrapped on x86_64-unknown-linux-gnu, testing in currently in
> progress.
> 
> I'll commit this unless somebody thinks it's a really bad idea
> at this point (I do not have numbers for the amount of memory
> we save - once we have fully compiled a function and we do not
> need its body for further inlining we throw it away and with it
> all leaked SSA names).

I'm not installing this now because I hit PR52198 with this change.
I'm instead queuing it for 4.8 where I can XFAIL the testcases
that are affected.

Richard.

> 2012-02-10  Richard Guenther  <rguenther@suse.de>
> 
> 	* tree-nrv.c (tree_nrv): Release VDEFs.
> 	* tree-inline.c (expand_call_inline): Likewise.
> 	* tree-sra.c (sra_modify_constructor_assign): Likewise.
> 	(sra_modify_assign): Likewise.
> 	* tree-vect-stmts.c (vect_remove_stores): Likewise.
> 	* tree-vect-loop.c (vect_transform_loop): Likewise.
> 	* tree-ssa-dom.c (optimize_stmt): Likewise.
> 	* tree-vect-slp.c (vect_schedule_slp): Likewise.
> 	* tree-ssa-math-opts.c (execute_cse_sincos): Likewise.
> 
> Index: gcc/tree-nrv.c
> ===================================================================
> --- gcc/tree-nrv.c	(revision 184088)
> +++ gcc/tree-nrv.c	(working copy)
> @@ -244,6 +244,7 @@ tree_nrv (void)
>  	    {
>  	      unlink_stmt_vdef (stmt);
>  	      gsi_remove (&gsi, true);
> +	      release_defs (stmt);
>  	    }
>  	  else
>  	    {
> Index: gcc/tree-inline.c
> ===================================================================
> --- gcc/tree-inline.c	(revision 184088)
> +++ gcc/tree-inline.c	(working copy)
> @@ -4002,6 +4002,9 @@ expand_call_inline (basic_block bb, gimp
>  
>    /* Unlink the calls virtual operands before replacing it.  */
>    unlink_stmt_vdef (stmt);
> +  if (gimple_vdef (stmt)
> +      && TREE_CODE (gimple_vdef (stmt)) == SSA_NAME)
> +    release_ssa_name (gimple_vdef (stmt));
>  
>    /* If the inlined function returns a result that we care about,
>       substitute the GIMPLE_CALL with an assignment of the return
> @@ -4043,7 +4046,7 @@ expand_call_inline (basic_block bb, gimp
>  	    }
>  	}
>        else
> -        gsi_remove (&stmt_gsi, true);
> +	gsi_remove (&stmt_gsi, true);
>      }
>  
>    if (purge_dead_abnormal_edges)
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c	(revision 184088)
> +++ gcc/tree-sra.c	(working copy)
> @@ -2858,6 +2858,7 @@ sra_modify_constructor_assign (gimple *s
>  	{
>  	  unlink_stmt_vdef (*stmt);
>  	  gsi_remove (gsi, true);
> +	  release_defs (*stmt);
>  	  return SRA_AM_REMOVED;
>  	}
>        else
> @@ -2881,6 +2882,7 @@ sra_modify_constructor_assign (gimple *s
>        init_subtree_with_zero (acc, gsi, false, loc);
>        unlink_stmt_vdef (*stmt);
>        gsi_remove (gsi, true);
> +      release_defs (*stmt);
>        return SRA_AM_REMOVED;
>      }
>    else
> @@ -3125,6 +3127,7 @@ sra_modify_assign (gimple *stmt, gimple_
>  	      gsi_next (gsi);
>  	      unlink_stmt_vdef (*stmt);
>  	      gsi_remove (&orig_gsi, true);
> +	      release_defs (*stmt);
>  	      sra_stats.deleted++;
>  	      return SRA_AM_REMOVED;
>  	    }
> @@ -3145,6 +3148,7 @@ sra_modify_assign (gimple *stmt, gimple_
>  	      gcc_assert (*stmt == gsi_stmt (*gsi));
>  	      unlink_stmt_vdef (*stmt);
>  	      gsi_remove (gsi, true);
> +	      release_defs (*stmt);
>  	      sra_stats.deleted++;
>  	      return SRA_AM_REMOVED;
>  	    }
> Index: gcc/tree-vect-stmts.c
> ===================================================================
> --- gcc/tree-vect-stmts.c	(revision 184088)
> +++ gcc/tree-vect-stmts.c	(working copy)
> @@ -5596,7 +5596,9 @@ vect_remove_stores (gimple first_stmt)
>  	next = STMT_VINFO_RELATED_STMT (stmt_info);
>        /* Free the attached stmt_vec_info and remove the stmt.  */
>        next_si = gsi_for_stmt (next);
> +      unlink_stmt_vdef (next);
>        gsi_remove (&next_si, true);
> +      release_defs (next);
>        free_stmt_vec_info (next);
>        next = tmp;
>      }
> Index: gcc/tree-vect-loop.c
> ===================================================================
> --- gcc/tree-vect-loop.c	(revision 184088)
> +++ gcc/tree-vect-loop.c	(working copy)
> @@ -5428,8 +5428,11 @@ vect_transform_loop (loop_vec_info loop_
>  	      else
>  		{
>  		  /* Free the attached stmt_vec_info and remove the stmt.  */
> -		  free_stmt_vec_info (gsi_stmt (si));
> +		  gcc_assert (stmt == gsi_stmt (si));
> +		  free_stmt_vec_info (stmt);
> +		  unlink_stmt_vdef (stmt);
>  		  gsi_remove (&si, true);
> +		  release_defs (stmt);
>  		  continue;
>  		}
>  	    }
> Index: gcc/tree-ssa-dom.c
> ===================================================================
> --- gcc/tree-ssa-dom.c	(revision 184088)
> +++ gcc/tree-ssa-dom.c	(working copy)
> @@ -2294,6 +2294,7 @@ optimize_stmt (basic_block bb, gimple_st
>  	      int lp_nr = lookup_stmt_eh_lp (stmt);
>  	      unlink_stmt_vdef (stmt);
>  	      gsi_remove (&si, true);
> +	      release_defs (stmt);
>  	      if (lp_nr != 0)
>  		{
>  		  bitmap_set_bit (need_eh_cleanup, bb->index);
> Index: gcc/tree-vect-slp.c
> ===================================================================
> --- gcc/tree-vect-slp.c	(revision 184088)
> +++ gcc/tree-vect-slp.c	(working copy)
> @@ -3008,7 +3008,9 @@ vect_schedule_slp (loop_vec_info loop_vi
>             store = STMT_VINFO_RELATED_STMT (vinfo_for_stmt (store));
>            /* Free the attached stmt_vec_info and remove the stmt.  */
>            gsi = gsi_for_stmt (store);
> +	  unlink_stmt_vdef (store);
>            gsi_remove (&gsi, true);
> +	  release_defs (store);
>            free_stmt_vec_info (store);
>          }
>      }
> Index: gcc/tree-ssa-math-opts.c
> ===================================================================
> --- gcc/tree-ssa-math-opts.c	(revision 184088)
> +++ gcc/tree-ssa-math-opts.c	(working copy)
> @@ -1430,6 +1430,8 @@ execute_cse_sincos (void)
>  		      gimple_set_location (new_stmt, loc);
>  		      unlink_stmt_vdef (stmt);
>  		      gsi_replace (&gsi, new_stmt, true);
> +		      if (gimple_vdef (stmt))
> +			release_ssa_name (gimple_vdef (stmt));
>  		    }
>  		  break;
>  
> @@ -1450,6 +1452,8 @@ execute_cse_sincos (void)
>  		      gimple_set_location (new_stmt, loc);
>  		      unlink_stmt_vdef (stmt);
>  		      gsi_replace (&gsi, new_stmt, true);
> +		      if (gimple_vdef (stmt))
> +			release_ssa_name (gimple_vdef (stmt));
>  		    }
>  		  break;
>  
> @@ -1465,6 +1469,8 @@ execute_cse_sincos (void)
>  		      gimple_set_location (new_stmt, loc);
>  		      unlink_stmt_vdef (stmt);
>  		      gsi_replace (&gsi, new_stmt, true);
> +		      if (gimple_vdef (stmt))
> +			release_ssa_name (gimple_vdef (stmt));
>  		    }
>  		  break;
>  
>

Patch

Index: tree-ssa.c
===================================================================
--- tree-ssa.c  (revision 184088)
+++ tree-ssa.c  (working copy)
@@ -944,6 +944,11 @@  verify_ssa (bool check_modified_stmt)
          if (!gimple_nop_p (stmt))
            {
              basic_block bb = gimple_bb (stmt);
+             if (!bb)
+               {
+                 error ("Unreleased SSA name");
+                 goto err;
+               }
              verify_def (bb, definition_block,
                          name, stmt, !is_gimple_reg (name));
 
and noticed we do not properly free a lot of virtual SSA names, leaking
both SSA names, stmts and operands they point to over time.

The following patch cures the cases I hit with running the tree-ssa.exp
testsuite.  More issues exist, and I guess we should make a more
verbose variant of the above the default in 4.8 to catch these issues
(there is a slight issue with this when verify_ssa is invoked during
loop transforms and the vectorizer has pattern stmts built that are
not in a basic block ... so maybe we should put the above into some
other verifier).

Bootstrapped on x86_64-unknown-linux-gnu, testing in currently in
progress.

I'll commit this unless somebody thinks it's a really bad idea
at this point (I do not have numbers for the amount of memory
we save - once we have fully compiled a function and we do not
need its body for further inlining we throw it away and with it
all leaked SSA names).

Thanks,
Richard.

2012-02-10  Richard Guenther  <rguenther@suse.de>

	* tree-nrv.c (tree_nrv): Release VDEFs.
	* tree-inline.c (expand_call_inline): Likewise.
	* tree-sra.c (sra_modify_constructor_assign): Likewise.
	(sra_modify_assign): Likewise.
	* tree-vect-stmts.c (vect_remove_stores): Likewise.
	* tree-vect-loop.c (vect_transform_loop): Likewise.
	* tree-ssa-dom.c (optimize_stmt): Likewise.
	* tree-vect-slp.c (vect_schedule_slp): Likewise.
	* tree-ssa-math-opts.c (execute_cse_sincos): Likewise.

Index: gcc/tree-nrv.c
===================================================================
--- gcc/tree-nrv.c	(revision 184088)
+++ gcc/tree-nrv.c	(working copy)
@@ -244,6 +244,7 @@  tree_nrv (void)
 	    {
 	      unlink_stmt_vdef (stmt);
 	      gsi_remove (&gsi, true);
+	      release_defs (stmt);
 	    }
 	  else
 	    {
Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c	(revision 184088)
+++ gcc/tree-inline.c	(working copy)
@@ -4002,6 +4002,9 @@  expand_call_inline (basic_block bb, gimp
 
   /* Unlink the calls virtual operands before replacing it.  */
   unlink_stmt_vdef (stmt);
+  if (gimple_vdef (stmt)
+      && TREE_CODE (gimple_vdef (stmt)) == SSA_NAME)
+    release_ssa_name (gimple_vdef (stmt));
 
   /* If the inlined function returns a result that we care about,
      substitute the GIMPLE_CALL with an assignment of the return
@@ -4043,7 +4046,7 @@  expand_call_inline (basic_block bb, gimp
 	    }
 	}
       else
-        gsi_remove (&stmt_gsi, true);
+	gsi_remove (&stmt_gsi, true);
     }
 
   if (purge_dead_abnormal_edges)
Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c	(revision 184088)
+++ gcc/tree-sra.c	(working copy)
@@ -2858,6 +2858,7 @@  sra_modify_constructor_assign (gimple *s
 	{
 	  unlink_stmt_vdef (*stmt);
 	  gsi_remove (gsi, true);
+	  release_defs (*stmt);
 	  return SRA_AM_REMOVED;
 	}
       else
@@ -2881,6 +2882,7 @@  sra_modify_constructor_assign (gimple *s
       init_subtree_with_zero (acc, gsi, false, loc);
       unlink_stmt_vdef (*stmt);
       gsi_remove (gsi, true);
+      release_defs (*stmt);
       return SRA_AM_REMOVED;
     }
   else
@@ -3125,6 +3127,7 @@  sra_modify_assign (gimple *stmt, gimple_
 	      gsi_next (gsi);
 	      unlink_stmt_vdef (*stmt);
 	      gsi_remove (&orig_gsi, true);
+	      release_defs (*stmt);
 	      sra_stats.deleted++;
 	      return SRA_AM_REMOVED;
 	    }
@@ -3145,6 +3148,7 @@  sra_modify_assign (gimple *stmt, gimple_
 	      gcc_assert (*stmt == gsi_stmt (*gsi));
 	      unlink_stmt_vdef (*stmt);
 	      gsi_remove (gsi, true);
+	      release_defs (*stmt);
 	      sra_stats.deleted++;
 	      return SRA_AM_REMOVED;
 	    }
Index: gcc/tree-vect-stmts.c
===================================================================
--- gcc/tree-vect-stmts.c	(revision 184088)
+++ gcc/tree-vect-stmts.c	(working copy)
@@ -5596,7 +5596,9 @@  vect_remove_stores (gimple first_stmt)
 	next = STMT_VINFO_RELATED_STMT (stmt_info);
       /* Free the attached stmt_vec_info and remove the stmt.  */
       next_si = gsi_for_stmt (next);
+      unlink_stmt_vdef (next);
       gsi_remove (&next_si, true);
+      release_defs (next);
       free_stmt_vec_info (next);
       next = tmp;
     }
Index: gcc/tree-vect-loop.c
===================================================================
--- gcc/tree-vect-loop.c	(revision 184088)
+++ gcc/tree-vect-loop.c	(working copy)
@@ -5428,8 +5428,11 @@  vect_transform_loop (loop_vec_info loop_
 	      else
 		{
 		  /* Free the attached stmt_vec_info and remove the stmt.  */
-		  free_stmt_vec_info (gsi_stmt (si));
+		  gcc_assert (stmt == gsi_stmt (si));
+		  free_stmt_vec_info (stmt);
+		  unlink_stmt_vdef (stmt);
 		  gsi_remove (&si, true);
+		  release_defs (stmt);
 		  continue;
 		}
 	    }
Index: gcc/tree-ssa-dom.c
===================================================================
--- gcc/tree-ssa-dom.c	(revision 184088)
+++ gcc/tree-ssa-dom.c	(working copy)
@@ -2294,6 +2294,7 @@  optimize_stmt (basic_block bb, gimple_st
 	      int lp_nr = lookup_stmt_eh_lp (stmt);
 	      unlink_stmt_vdef (stmt);
 	      gsi_remove (&si, true);
+	      release_defs (stmt);
 	      if (lp_nr != 0)
 		{
 		  bitmap_set_bit (need_eh_cleanup, bb->index);
Index: gcc/tree-vect-slp.c
===================================================================
--- gcc/tree-vect-slp.c	(revision 184088)
+++ gcc/tree-vect-slp.c	(working copy)
@@ -3008,7 +3008,9 @@  vect_schedule_slp (loop_vec_info loop_vi
            store = STMT_VINFO_RELATED_STMT (vinfo_for_stmt (store));
           /* Free the attached stmt_vec_info and remove the stmt.  */
           gsi = gsi_for_stmt (store);
+	  unlink_stmt_vdef (store);
           gsi_remove (&gsi, true);
+	  release_defs (store);
           free_stmt_vec_info (store);
         }
     }
Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c	(revision 184088)
+++ gcc/tree-ssa-math-opts.c	(working copy)
@@ -1430,6 +1430,8 @@  execute_cse_sincos (void)
 		      gimple_set_location (new_stmt, loc);
 		      unlink_stmt_vdef (stmt);
 		      gsi_replace (&gsi, new_stmt, true);
+		      if (gimple_vdef (stmt))
+			release_ssa_name (gimple_vdef (stmt));
 		    }
 		  break;
 
@@ -1450,6 +1452,8 @@  execute_cse_sincos (void)
 		      gimple_set_location (new_stmt, loc);
 		      unlink_stmt_vdef (stmt);
 		      gsi_replace (&gsi, new_stmt, true);
+		      if (gimple_vdef (stmt))
+			release_ssa_name (gimple_vdef (stmt));
 		    }
 		  break;
 
@@ -1465,6 +1469,8 @@  execute_cse_sincos (void)
 		      gimple_set_location (new_stmt, loc);
 		      unlink_stmt_vdef (stmt);
 		      gsi_replace (&gsi, new_stmt, true);
+		      if (gimple_vdef (stmt))
+			release_ssa_name (gimple_vdef (stmt));
 		    }
 		  break;