[ was: Re: Expand oacc kernels after pass_fre ]
On 04/06/15 18:02, Tom de Vries wrote:
Please move this out of the class body.
Fixed and committed (ommitting patch as trivial).
+ {
+ unsigned res = execute_expand_omp ();
+
+ /* 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. */
+ 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);
+ 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);
+ }
+ }
+
+ return res;
+ }
This patch implements the TODO.
The cause of the problems is that in replace_ssa_name, we create a new
ssa_name with the def stmt of the old ssa_name, but do not reset the def
stmt of the old ssa_name, leaving the ssa_name in the original function
having a def stmt in the split-off function.
[ And if we don't do anything about that, at some point in another pass
we use 'gimple_bb (SSA_NAME_DEF_STMT (name))->index' (a bb index in the
split-off function) as an index into an array with as length the number
of bbs in the original function. So the index may be out of bounds. ]
This patch fixes that by making sure we reset the def stmt to NULL. This
means we can simplify release_dangling_ssa_names to just test for NULL
def stmts.
Default defs are skipped by release_ssa_name, so setting the def stmt
for default defs to NULL does not result in the name being released, but
in an ssa-verification error. So instead, we keep the def stmt nop, and
create a new nop for the copy in the split-off function.
[ The default def bit seems only to be triggered for the default def
created by expand_omp_target:
...
/* If we are in ssa form, we must load the value from the default
definition of the argument. That should not be defined now,
since the argument is not used uninitialized. */
gcc_assert (ssa_default_def (cfun, arg) == NULL);
narg = make_ssa_name (arg, gimple_build_nop ());
set_ssa_default_def (cfun, arg, narg);
...
]
Bootstrapped and reg-tested on x86_64.
Committed to gomp-4_0-branch.
Thanks,
- Tom
Fix release_dangling_ssa_names
2015-08-05 Tom de Vries <t...@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 | 35 +++++++++++------------------------
gcc/tree-cfg.c | 17 ++++++++++++++++-
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 0ebbbe1..cd2076f 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -10349,11 +10349,10 @@ 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. */
+/* 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. */
static void
release_dangling_ssa_names (void)
@@ -10366,26 +10365,14 @@ release_dangling_ssa_names (void)
continue;
gimple stmt = SSA_NAME_DEF_STMT (name);
- 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 (stmt != NULL)
+ continue;
- if (!found)
- {
- if (dump_file)
- fprintf (dump_file, "Released dangling ssa name %u\n", i);
- release_ssa_name (name);
- }
+ 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);
}
}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index cb9fe6d..6a00b25 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6467,8 +6467,17 @@ replace_ssa_name (tree name, hash_map<tree, tree> *vars_map,
if (decl)
{
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. */
+ gimple def_stmt = (SSA_NAME_IS_DEFAULT_DEF (name)
+ ? gimple_build_nop ()
+ : SSA_NAME_DEF_STMT (name));
new_name = make_ssa_name_fn (DECL_STRUCT_FUNCTION (to_context),
- decl, SSA_NAME_DEF_STMT (name));
+ decl, def_stmt);
if (SSA_NAME_IS_DEFAULT_DEF (name))
set_ssa_default_def (DECL_STRUCT_FUNCTION (to_context),
decl, new_name);
@@ -6478,6 +6487,12 @@ replace_ssa_name (tree name, hash_map<tree, tree> *vars_map,
name, SSA_NAME_DEF_STMT (name));
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
