On Tue, 11 Aug 2015, Tom de Vries wrote:
> [ 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?
Ok.
Thanks,
Richard.
