This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [committed, gomp4] Fix release_dangling_ssa_names
- From: Tom de Vries <Tom_deVries at mentor dot com>
- To: Richard Biener <rguenther at suse dot de>
- Cc: Thomas Schwinge <thomas at codesourcery dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>
- Date: Wed, 5 Aug 2015 12:48:37 +0200
- Subject: Re: [committed, gomp4] Fix release_dangling_ssa_names
- Authentication-results: sourceware.org; auth=none
- References: <546743BC dot 5070804 at mentor dot com> <54678B3D dot 8020009 at mentor dot com> <54730EE7 dot 40000 at mentor dot com> <5474665A dot 4070101 at mentor dot com> <87iocp1d8o dot fsf at kepler dot schwinge dot homeip dot net> <alpine dot LSU dot 2 dot 11 dot 1504220932150 dot 20496 at zhemvz dot fhfr dot qr> <557076A5 dot 7050207 at mentor dot com> <55C1BA11 dot 70008 at mentor dot com> <alpine dot LSU dot 2 dot 11 dot 1508050928320 dot 19642 at zhemvz dot fhfr dot qr> <55C1CDAF dot 8050402 at mentor dot com> <alpine dot LSU dot 2 dot 11 dot 1508051116450 dot 19642 at zhemvz dot fhfr dot qr>
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.
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.
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.
Thanks,
- Tom