[committed, gomp4] Fix release_dangling_ssa_names

[Richard Biener]

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

+
+      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.

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).

Trunk doesn't have release_dangling_ssa_names it seems but I think
it belongs to move_sese_region_to_fn and not to omp-low.c and it
could also just walk the d->vars_map replace_ssa_name fills to
iterate over the removal candidates (and if the situation of
moving uses but not defs cannot happen you don't need any
SSA_NAME_DEF_STMT dance either).

Thanks,
Richard.

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)