[gomp4,committed] Remove release_dangling_ssa_names

[Tom de Vries <Tom_deVries@mentor.com>]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

[ was: Re: [PATCH] Don't create superfluous parm in expand_omp_taskreg ]
On 24/09/15 11:02, Thomas Schwinge wrote:
> Hi Tom!
>
> On Thu, 24 Sep 2015 08:36:27 +0200, Tom de Vries<Tom_deVries@mentor.com>  wrote:
>> >On 24/09/15 08:23, Thomas Schwinge wrote:
>>> > >On Tue, 11 Aug 2015 20:53:39 +0200, Tom de Vries<Tom_deVries@mentor.com>  wrote:
>>>> > >>Don't create superfluous parm in expand_omp_taskreg
>>>> > >>
>>>> > >>2015-08-11  Tom de Vries<tom@codesourcery.com>
>>>> > >>
>>>> > >>	* omp-low.c (expand_omp_taskreg): If in ssa, set rhs of parcopy stmt to
>>>> > >>	parm_decl, rather than generating a dummy default def in cfun.
>>>> > >>	* tree-cfg.c (replace_ssa_name): Assume no default defs.  Make sure
>>>> > >>	ssa_name from cfun and child_fn do not share a stmt as def stmt.
>>>> > >>	(move_stmt_op): Handle PARM_DECl.
>>>> > >>	(gather_ssa_name_hash_map_from): New function.
>>>> > >>	(move_sese_region_to_fn): Add default defs for function params, and add
>>>> > >>	them to vars_map.  Release copied ssa names.
>>>> > >>	* tree-cfg.h (gather_ssa_name_hash_map_from): Declare.
>>> > >
>>> > >Do I understand correct that with this change present on trunk (which I'm
>>> > >currently merging into gomp-4_0-branch), the changes you've earlier done
>>> > >on gomp-4_0-branch to gcc/omp-low.c:release_dangling_ssa_names,
>>> > >gcc/tree-cfg.c:replace_ssa_name, should now be reverted?  That is, how
>>> > >much of the following patches can be reverted now (listed backwards in
>>> > >time)?
>> >
>> >indeed, in the above commit we release the dangling ssa names in
>> >move_sese_region_to_fn. So after committing this patch to the
>> >gomp-4_0-branch, the call to release_dangling_ssa_names is no longer
>> >necessary, and the function release_dangling_ssa_names can be removed.

<SNIP>

>      <tschwinge> Well, I'm asking because in my merge tree, I'm running
>        into an assertion that you added there -- not sure yet whether I've
>        done something wrong, though.

The source of the problem was in expand_omp_target, which needed similar 
changes as expand_omp_taskreg got in the "Don't create superfluous parm 
in expand_omp_taskreg" patch.

Now that the merge ( 
https://gcc.gnu.org/viewcvs/gcc/branches/gomp-4_0-branch/gcc/omp-low.c?limit_changes=0&r1=228091&r2=228090&pathrev=228091 
) contains that change, I've committed these two patches to gomp-4_0-branch:
- Revert "Fix release_dangling_ssa_names"
   (Reverting an earlier attempt to handle the
   release_dangling_ssa_names TODO, which was committed to the
   gomp-4_0-branch)
- Remove release_dangling_ssa_names

Thanks,
- Tom


[-- Attachment #2: 0001-Revert-Fix-release_dangling_ssa_names.patch --]
[-- Type: text/x-patch, Size: 3457 bytes --]

Revert "Fix release_dangling_ssa_names"

2015-09-24  Tom de Vries  <tom@codesourcery.com>

	Revert:
	2015-08-05  Tom de Vries  <tom@codesourcery.com>

	* omp-low.c (release_dangling_ssa_names): Release SSA_NAMEs with NULL
	def stmt.
	* tree-cfg.c (replace_ssa_name): Don't move default def nops.  Set def
	stmt of unused SSA_NAME to NULL.
---
 gcc/omp-low.c  | 33 ++++++++++++++++++++++++---------
 gcc/tree-cfg.c | 12 ------------
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index a72db53..04a60ab 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -10541,10 +10541,11 @@ make_pass_expand_omp (gcc::context *ctxt)
   return new pass_expand_omp (ctxt);
 }
 
-/* After running pass_expand_omp_ssa to expand the oacc kernels directive, we
-   are left in the original function with anonymous SSA_NAMEs, with a NULL
-   defining statement.  This function finds those SSA_NAMEs and releases
-   them.  */
+/* After running pass_expand_omp_ssa to expand the oacc kernels
+   directive, we are left in the original function with anonymous
+   SSA_NAMEs, with a defining statement that has been deleted.  This
+   pass finds those SSA_NAMEs and releases them.
+   TODO: Either fix this elsewhere, or make the fix unnecessary.  */
 
 static void
 release_dangling_ssa_names (void)
@@ -10559,12 +10560,26 @@ release_dangling_ssa_names (void)
       gimple *stmt = SSA_NAME_DEF_STMT (name);
       if (stmt != NULL)
 	continue;
+      bool found = false;
 
-      release_ssa_name (name);
-      gcc_assert (SSA_NAME_IN_FREE_LIST (name));
-      if (dump_file
-	  && (dump_flags & TDF_DETAILS))
-	fprintf (dump_file, "Released dangling ssa name %u\n", i);
+      ssa_op_iter op_iter;
+      def_operand_p def_p;
+      FOR_EACH_PHI_OR_STMT_DEF (def_p, stmt, op_iter, SSA_OP_ALL_DEFS)
+	{
+	  tree def = DEF_FROM_PTR (def_p);
+	  if (def == name)
+	    {
+	      found = true;
+	      break;
+	    }
+	}
+
+      if (!found)
+	{
+	  if (dump_file)
+	    fprintf (dump_file, "Released dangling ssa name %u\n", i);
+	  release_ssa_name (name);
+	}
     }
 }
 
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index cd7a4b4..a3c3b20 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6422,12 +6422,6 @@ replace_ssa_name (tree name, hash_map<tree, tree> *vars_map,
 	{
 	  gcc_assert (!SSA_NAME_IS_DEFAULT_DEF (name));
 	  replace_by_duplicate_decl (&decl, vars_map, to_context);
-	  /* If name is a default def, then we don't move the defining stmt
-	     (which is a nop).  Because (1) the nop doesn't belong to the sese
-	     region, and (2) while setting the def stmt of name to NULL would
-	     trigger release_ssa_name in release_dangling_ssa_names, it wouldn't
-	     be released since it's a default def, and subsequently cause an
-	     ssa verification failure.  */
 	  new_name = make_ssa_name_fn (DECL_STRUCT_FUNCTION (to_context),
 				       decl, SSA_NAME_DEF_STMT (name));
 	  if (SSA_NAME_IS_DEFAULT_DEF (name))
@@ -6443,12 +6437,6 @@ replace_ssa_name (tree name, hash_map<tree, tree> *vars_map,
       SSA_NAME_DEF_STMT (name) = NULL;
 
       vars_map->put (name, new_name);
-
-      if (!SSA_NAME_IS_DEFAULT_DEF (name))
-	/* The statement has been moved to the child function.  It no longer
-	   defines name in the original function.  Mark the def stmt NULL, and
-	   let release_dangling_ssa_names deal with it.  */
-	SSA_NAME_DEF_STMT (name) = NULL;
     }
   else
     new_name = *loc;
-- 
1.9.1


[-- Attachment #3: 0002-Remove-release_dangling_ssa_names.patch --]
[-- Type: text/x-patch, Size: 1966 bytes --]

Remove release_dangling_ssa_names

2015-09-24  Tom de Vries  <tom@codesourcery.com>

	* omp-low.c (release_dangling_ssa_names): Remove.
	(pass_omp_expand_ssa::execute): Remove call to
	release_dangling_ssa_names.
---
 gcc/omp-low.c | 46 +---------------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 04a60ab..6bdfaa2 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -10541,48 +10541,6 @@ make_pass_expand_omp (gcc::context *ctxt)
   return new pass_expand_omp (ctxt);
 }
 
-/* After running pass_expand_omp_ssa to expand the oacc kernels
-   directive, we are left in the original function with anonymous
-   SSA_NAMEs, with a defining statement that has been deleted.  This
-   pass finds those SSA_NAMEs and releases them.
-   TODO: Either fix this elsewhere, or make the fix unnecessary.  */
-
-static void
-release_dangling_ssa_names (void)
-{
-  unsigned int i;
-  for (i = 1; i < num_ssa_names; ++i)
-    {
-      tree name = ssa_name (i);
-      if (name == NULL_TREE)
-	continue;
-
-      gimple *stmt = SSA_NAME_DEF_STMT (name);
-      if (stmt != NULL)
-	continue;
-      bool found = false;
-
-      ssa_op_iter op_iter;
-      def_operand_p def_p;
-      FOR_EACH_PHI_OR_STMT_DEF (def_p, stmt, op_iter, SSA_OP_ALL_DEFS)
-	{
-	  tree def = DEF_FROM_PTR (def_p);
-	  if (def == name)
-	    {
-	      found = true;
-	      break;
-	    }
-	}
-
-      if (!found)
-	{
-	  if (dump_file)
-	    fprintf (dump_file, "Released dangling ssa name %u\n", i);
-	  release_ssa_name (name);
-	}
-    }
-}
-
 namespace {
 
 const pass_data pass_data_expand_omp_ssa =
@@ -10613,9 +10571,7 @@ public:
     }
   virtual unsigned int execute (function *)
     {
-      unsigned res = execute_expand_omp ();
-      release_dangling_ssa_names ();
-      return res;
+      return execute_expand_omp ();
     }
   opt_pass * clone () { return new pass_expand_omp_ssa (m_ctxt); }
 
-- 
1.9.1