[PATCH] Don't create superfluous parm in expand_omp_taskreg
Thomas Schwinge
thomas@codesourcery.com
Thu Sep 24 06:36:00 GMT 2015
Hi Tom!
On Tue, 11 Aug 2015 20:53:39 +0200, Tom de Vries <Tom_deVries@mentor.com> 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?
>
> Thanks,
> - Tom
> Don't create superfluous parm in expand_omp_taskreg
>
> 2015-08-11 Tom de Vries <tom@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.
Do I understand correct that with this change present on trunk (which I'm
currently merging into gomp-4_0-branch), the changes you've earlier done
on gomp-4_0-branch to gcc/omp-low.c:release_dangling_ssa_names,
gcc/tree-cfg.c:replace_ssa_name, should now be reverted? That is, how
much of the following patches can be reverted now (listed backwards in
time)?
commit 6befb84f4c0157a4cdf66cfaf64e457180f9a7fa
Author: vries <vries@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Wed Aug 5 06:01:08 2015 +0000
Fix release_dangling_ssa_names
2015-08-05 Tom de Vries <tom@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.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@226608 138bc75d-0d04-0410-961f-82ee72b054a4
commit 0cf67438bd87e5a6ec063e90da0ea20801bda54c
Author: vries <vries@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Thu Jun 4 15:47:09 2015 +0000
Add release_dangling_ssa_names
2015-06-04 Tom de Vries <tom@codesourcery.com>
* omp-low.c (release_dangling_ssa_names): Factor out of ...
(pass_expand_omp_ssa::execute): ... here.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@224130 138bc75d-0d04-0410-961f-82ee72b054a4
commit 93557ac5e30c26ee1a3d1255e31265b287171a0d
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Tue Apr 21 19:37:19 2015 +0000
Expand oacc kernels after pass_fre
gcc/
* omp-low.c: Include gimple-pretty-print.h.
(release_first_vuse_in_edge_dest): New function.
(expand_omp_target): When not in ssa, don't split off oacc kernels
region, clear PROP_gimple_eomp in cfun->curr_properties to force later
expanssion, and add GOACC_kernels_internal call.
When in ssa, split off oacc kernels and convert GOACC_kernels_internal
into GOACC_kernels call. Handle ssa-code.
(pass_data_expand_omp): Don't set PROP_gimple_eomp unconditionally in
properties_provided field.
(pass_expand_omp::execute): Set PROP_gimple_eomp in
cfun->curr_properties tentatively.
(pass_data_expand_omp_ssa): Add TODO_remove_unused_locals to
todo_flags_finish field.
(pass_expand_omp_ssa::execute): Release dangling SSA_NAMEs after calling
execute_expand_omp.
(gimple_stmt_ssa_operand_references_var_p)
(gimple_stmt_omp_data_i_init_p): New function.
* omp-low.h (gimple_stmt_omp_data_i_init_p): Declare.
* passes.def: Add pass_expand_omp_ssa after pass_fre. Add
pass_expand_omp_ssa after pass_all_early_optimizations.
* tree-ssa-ccp.c: Include omp-low.h.
(surely_varying_stmt_p, ccp_visit_stmt): Handle .omp_data_i init
conservatively.
* tree-ssa-forwprop.c: Include omp-low.h.
(pass_forwprop::execute): Handle .omp_data_i init conservatively.
* tree-ssa-sccvn.c: Include omp-low.h.
(visit_use): Handle .omp_data_i init conservatively.
* cgraph.c (cgraph_node::release_body): Don't release offloadable
functions.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@222279 138bc75d-0d04-0410-961f-82ee72b054a4
Grüße,
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 472 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20150924/ff5a5cad/attachment.sig>
More information about the Gcc-patches
mailing list