[ was: Re: [committed, gomp4] Fix release_dangling_ssa_names ]
On 05/08/15 13:13, Richard Biener wrote:
On Wed, 5 Aug 2015, Tom de Vries wrote:
On 05/08/15 11:30, Richard Biener wrote:
On Wed, 5 Aug 2015, Tom de Vries wrote:
On 05/08/15 09:29, Richard Biener wrote:
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.
Not sure if I understand the problem correctly but why are you not
simply
releasing the SSA name when you remove its definition?
In move_sese_region_to_fn we move a region of blocks from one function to
another, bit by bit.
When we encounter an ssa_name as def or use in the region, we:
- generate a new ssa_name,
- set the def stmt of the old name as def stmt of the new name, and
- add a mapping from the old to the new name.
The next time we encounter the same ssa_name in another statement, we find
it
in the map.
If we release the old ssa name, we effectively create statements with
operands
in the free-list. The first point where that cause breakage, is in
walk_gimple_op, which expects the TREE_TYPE of the lhs of an assign to be
defined, which is not the case if it's in the free-list:
...
case GIMPLE_ASSIGN:
/* Walk the RHS operands. If the LHS is of a non-renamable type or
is a register variable, we may use a COMPONENT_REF on the RHS.*/
if (wi)
{
tree lhs = gimple_assign_lhs (stmt);
wi->val_only
= (is_gimple_reg_type (TREE_TYPE (lhs)) && !is_gimple_reg (lhs))
|| gimple_assign_rhs_class (stmt) != GIMPLE_SINGLE_RHS;
}
...
Hmm, ok, probably because the stmt moving doesn't happen in DOM
order (move defs before uses). But
There seems to be similar code for the rhs, so I don't think changing the
order would fix anything.
+
+ 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;
applies also to uses - I don't see why it couldn't happen that you
move a use but not its def (the def would be a parameter to the
split-out function). You'd wreck the IL of the source function this way.
If you first move a use, you create a mapping. When you encounter the def, you
use the mapping. Indeed, if the def is a default def, we don't encounter the
def. Which is why we create a nop as defining def for those cases. The default
def in the source function still has a defining nop, and has no uses anymore.
I don't understand what is broken here.
If you never encounter the DEF then it's broken. Say, if for
foo(int a)
{
int b = a;
if (b)
{
< code using b >
}
}
you move < code using b > to a function. Then the def is still in
foo but you create a mapping for its use(s). Clearly the outlining
process in this case has to pass b as parameter to the outlined
function, something that may not happen currently.
Ah, I see. Indeed, this is a situation that is assumed not to happen.
It would probably be cleaner to separate the def and use remapping
to separate functions and record on whether we saw a def or not.
Right, or some other means to detect this situation, say when copying
the def stmt in replace_ssa_name, check whether it's part of the sese
region.
I think that the whole dance of actually moving things instead of
just copying it isn't worth the extra maintainance (well, if we already
have a machinery duplicating a SESE region to another function - I
suppose gimple_duplicate_sese_region could be trivially changed to
support that).
I'll mention that as todo. For now, I think the fastest way to get a working
version is to fix move_sese_region_to_fn.
Sure.
Trunk doesn't have release_dangling_ssa_names it seems
Yep, I only ran into this trouble for the kernels region handling. But I don't
exclude the possibility it could happen for trunk as well.
but I think
it belongs to move_sese_region_to_fn and not to omp-low.c
Makes sense indeed.
and it
could also just walk the d->vars_map replace_ssa_name fills to
iterate over the removal candidates
Agreed, I suppose in general that's a win over iterating over all the ssa
names.
(and if the situation of
moving uses but not defs cannot happen you don't need any
SSA_NAME_DEF_STMT dance either).
I'd prefer to keep the SSA_NAME_DEF_STMT () = NULL bit. It makes sure a stmt
is the defining stmt of only one ssa-name at all times.
I'll prepare a patch for trunk then.
This patch fixes two problems with expand_omp_taskreg:
- it makes sure we don't generate a dummy default def in the original
function (which we cannot get rid of afterwards, given that it's a
default def).
- it releases ssa-names in the original function that have defining
statements that have been moved to the split-off function.
Bootstrapped and reg-tested on x86_64.
OK for trunk?
Thanks,
- Tom
Don't create superfluous parm in expand_omp_taskreg
2015-08-11 Tom de Vries <t...@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.
---
gcc/omp-low.c | 20 ++++++++++----------
gcc/tree-cfg.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
gcc/tree-cfg.h | 1 +
3 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index c1dc919..6f32a4a 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -5417,7 +5417,7 @@ expand_omp_taskreg (struct omp_region *region)
basic_block entry_succ_bb
= single_succ_p (entry_bb) ? single_succ (entry_bb)
: FALLTHRU_EDGE (entry_bb)->dest;
- tree arg, narg;
+ tree arg;
gimple parcopy_stmt = NULL;
for (gsi = gsi_start_bb (entry_succ_bb); ; gsi_next (&gsi))
@@ -5462,15 +5462,15 @@ expand_omp_taskreg (struct omp_region *region)
}
else
{
- /* 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);
- /* ?? Is setting the subcode really necessary ?? */
- gimple_omp_set_subcode (parcopy_stmt, TREE_CODE (narg));
- gimple_assign_set_rhs1 (parcopy_stmt, narg);
+ tree lhs = gimple_assign_lhs (parcopy_stmt);
+ gcc_assert (SSA_NAME_VAR (lhs) == arg);
+ /* We'd like to set the rhs to the default def in the child_fn,
+ but it's too early to create ssa names in the child_fn.
+ Instead, we set the rhs to the parm. In
+ move_sese_region_to_fn, we introduce a default def for the
+ parm, map the parm to it's default def, and once we encounter
+ this stmt, replace the parm with the default def. */
+ gimple_assign_set_rhs1 (parcopy_stmt, arg);
update_stmt (parcopy_stmt);
}
}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 588ab69..8afa5fb 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6422,17 +6422,19 @@ replace_ssa_name (tree name, hash_map<tree, tree> *vars_map,
tree decl = SSA_NAME_VAR (name);
if (decl)
{
+ gcc_assert (!SSA_NAME_IS_DEFAULT_DEF (name));
replace_by_duplicate_decl (&decl, vars_map, to_context);
new_name = make_ssa_name_fn (DECL_STRUCT_FUNCTION (to_context),
decl, SSA_NAME_DEF_STMT (name));
- if (SSA_NAME_IS_DEFAULT_DEF (name))
- set_ssa_default_def (DECL_STRUCT_FUNCTION (to_context),
- decl, new_name);
}
else
new_name = copy_ssa_name_fn (DECL_STRUCT_FUNCTION (to_context),
name, SSA_NAME_DEF_STMT (name));
+ /* Now that we've used the def stmt to define new_name, make sure it
+ doesn't define name anymore. */
+ SSA_NAME_DEF_STMT (name) = NULL;
+
vars_map->put (name, new_name);
}
else
@@ -6484,6 +6486,9 @@ move_stmt_op (tree *tp, int *walk_subtrees, void *data)
{
if (TREE_CODE (t) == SSA_NAME)
*tp = replace_ssa_name (t, p->vars_map, p->to_context);
+ else if (TREE_CODE (t) == PARM_DECL
+ && gimple_in_ssa_p (cfun))
+ *tp = *(p->vars_map->get (t));
else if (TREE_CODE (t) == LABEL_DECL)
{
if (p->new_label_map)
@@ -6994,6 +6999,19 @@ verify_sese (basic_block entry, basic_block exit, vec<basic_block> *bbs_p)
BITMAP_FREE (bbs);
}
+/* If FROM is an SSA_NAME, mark the version in bitmap DATA. */
+
+bool
+gather_ssa_name_hash_map_from (tree const &from, tree const &, void *data)
+{
+ bitmap release_names = (bitmap)data;
+
+ if (TREE_CODE (from) != SSA_NAME)
+ return true;
+
+ bitmap_set_bit (release_names, SSA_NAME_VERSION (from));
+ return true;
+}
/* Move a single-entry, single-exit region delimited by ENTRY_BB and
EXIT_BB to function DEST_CFUN. The whole region is replaced by a
@@ -7191,6 +7209,14 @@ move_sese_region_to_fn (struct function *dest_cfun, basic_block entry_bb,
d.eh_map = eh_map;
d.remap_decls_p = true;
+ if (gimple_in_ssa_p (cfun))
+ for (tree arg = DECL_ARGUMENTS (d.to_context); arg; arg = DECL_CHAIN (arg))
+ {
+ tree narg = make_ssa_name_fn (dest_cfun, arg, gimple_build_nop ());
+ set_ssa_default_def (dest_cfun, arg, narg);
+ vars_map.put (arg, narg);
+ }
+
FOR_EACH_VEC_ELT (bbs, i, bb)
{
/* No need to update edge counts on the last block. It has
@@ -7248,6 +7274,19 @@ move_sese_region_to_fn (struct function *dest_cfun, basic_block entry_bb,
if (eh_map)
delete eh_map;
+ if (gimple_in_ssa_p (cfun))
+ {
+ /* We need to release ssa-names in a defined order, so first find them,
+ and then iterate in ascending version order. */
+ bitmap release_names = BITMAP_ALLOC (NULL);
+ vars_map.traverse<void *, gather_ssa_name_hash_map_from> (release_names);
+ bitmap_iterator bi;
+ unsigned i;
+ EXECUTE_IF_SET_IN_BITMAP (release_names, 0, i, bi)
+ release_ssa_name (ssa_name (i));
+ BITMAP_FREE (release_names);
+ }
+
/* Rewire the entry and exit blocks. The successor to the entry
block turns into the successor of DEST_FN's ENTRY_BLOCK_PTR in
the child function. Similarly, the predecessor of DEST_FN's
diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
index 6c4b1d9..4bd6fcf 100644
--- a/gcc/tree-cfg.h
+++ b/gcc/tree-cfg.h
@@ -75,6 +75,7 @@ extern bool gimple_duplicate_sese_tail (edge, edge, basic_block *, unsigned,
extern void gather_blocks_in_sese_region (basic_block entry, basic_block exit,
vec<basic_block> *bbs_p);
extern void verify_sese (basic_block, basic_block, vec<basic_block> *);
+extern bool gather_ssa_name_hash_map_from (tree const &, tree const &, void *);
extern basic_block move_sese_region_to_fn (struct function *, basic_block,
basic_block, tree);
extern void dump_function_to_file (tree, FILE *, int);
--
1.9.1