Message ID | 55CA44B3.2000409@mentor.com |
---|---|
State | New |
Headers | show |
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.
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
On 24/09/15 08:23, Thomas Schwinge wrote: > Hi Tom! > > On Tue, 11 Aug 2015 20:53:39 +0200, Tom de Vries <Tom_deVries@mentor.com> wrote: >> 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)? > Hi Thomas, indeed, in the above commit we release the dangling ssa names in move_sese_region_to_fn. So after committing this patch to the gomp-4_0-branch, the call to release_dangling_ssa_names is no longer necessary, and the function release_dangling_ssa_names can be removed. Thanks, - Tom > 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 >
Hi Tom! On Thu, 24 Sep 2015 08:36:27 +0200, Tom de Vries <Tom_deVries@mentor.com> wrote: > On 24/09/15 08:23, Thomas Schwinge wrote: > > On Tue, 11 Aug 2015 20:53:39 +0200, Tom de Vries <Tom_deVries@mentor.com> wrote: > >> 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)? > > indeed, in the above commit we release the dangling ssa names in > move_sese_region_to_fn. So after committing this patch to the > gomp-4_0-branch, the call to release_dangling_ssa_names is no longer > necessary, and the function release_dangling_ssa_names can be removed. From IRC: <tschwinge> vries: Are you totally busy right now, or could you spend an hour on backporting to gomp-4_0-branch your trunk commit that I mentorioned earlier today? <vries> tschwinge: shouldn't be a problem <tschwinge> veWell, I'm asking because in my merge tree, I'm running into an assertion that you added there -- not sure yet whether I've done something wrong, though. <tschwinge> vries: ^ <vries> tschwinge: it would be useful for me to know which assertion So far, I only looked at libgomp test results, and there, none of the OpenMP but quite a number of the OpenACC tests fail as follows, for example libgomp.oacc-c-c++-common/kernels-1.c: [...]/build-gcc/gcc/xgcc -B[...]/build-gcc/gcc/ [...]/source-gcc/libgomp/testsuite/libgomp.oacc-c/../libgomp.oacc-c-c++-common/kernels-1.c -B[...]/build-gcc/x86_64-pc-linux-gnu/./libgomp/ -B[...]/build-gcc/x86_64-pc-linux-gnu/./libgomp/.libs -I[...]/build-gcc/x86_64-pc-linux-gnu/./libgomp -I[...]/source-gcc/libgomp/testsuite/../../include -I[...]/source-gcc/libgomp/testsuite/.. -I/usr/local/cuda-5.5/targets/x86_64-linux/include -fmessage-length=0 -fno-diagnostics-show-caret -fdiagnostics-color=never -B[...]/install/offload-nvptx-none/libexec/gcc/x86_64-pc-linux-gnu/6.0.0 -B[...]/install/offload-nvptx-none/bin -B[...]/install/offload-x86_64-intelmicemul-linux-gnu/libexec/gcc/x86_64-pc-linux-gnu/6.0.0 -B[...]/install/offload-x86_64-intelmicemul-linux-gnu/bin -fopenacc -I[...]/source-gcc/libgomp/testsuite/libgomp.oacc-c-c++-common -DACC_DEVICE_TYPE_nvidia=1 -DACC_MEM_SHARED=0 -foffload=nvptx-none -O2 -L[...]/build-gcc/x86_64-pc-linux-gnu/./libgomp/.libs -lm -o ./kernels-1.exe In file included from [...]/source-gcc/libgomp/testsuite/libgomp.oacc-c/../libgomp.oacc-c-c++-common/kernels-1.c:6:0: [...]/source-gcc/libgomp/testsuite/libgomp.oacc-c/../libgomp.oacc-c-c++-common/data-clauses.h: In function 'main': [...]/source-gcc/libgomp/testsuite/libgomp.oacc-c/../libgomp.oacc-c-c++-common/data-clauses.h:182:9: internal compiler error: in replace_ssa_name, at tree-cfg.c:6423 0xb0518a replace_ssa_name [...]/source-gcc/gcc/tree-cfg.c:6423 0xb05407 move_stmt_op [...]/source-gcc/gcc/tree-cfg.c:6501 0xd75e43 walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) [...]/source-gcc/gcc/tree.c:11341 0x89832c walk_gimple_op(gimple*, tree_node* (*)(tree_node**, int*, void*), walk_stmt_info*) [...]/source-gcc/gcc/gimple-walk.c:204 0x898814 walk_gimple_stmt(gimple_stmt_iterator*, tree_node* (*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, int*, void*), walk_stmt_info*) [...]/source-gcc/gcc/gimple-walk.c:562 0xb088e3 move_block_to_fn [...]/source-gcc/gcc/tree-cfg.c:6774 0xb088e3 move_sese_region_to_fn(function*, basic_block_def*, basic_block_def*, tree_node*) [...]/source-gcc/gcc/tree-cfg.c:7238 0x9c1133 expand_omp_target [...]/source-gcc/gcc/omp-low.c:9802 0x9c3a1c expand_omp [...]/source-gcc/gcc/omp-low.c:10240 0x9cc3fe execute_expand_omp [...]/source-gcc/gcc/omp-low.c:10486 0x9cc568 execute [...]/source-gcc/gcc/omp-low.c:10609 source-gcc/gcc/tree-cfg.c: [...] 6405 /* Creates an ssa name in TO_CONTEXT equivalent to NAME. 6406 VARS_MAP maps old ssa names and var_decls to the new ones. */ 6407 6408 static tree 6409 replace_ssa_name (tree name, hash_map<tree, tree> *vars_map, 6410 tree to_context) 6411 { 6412 tree new_name; 6413 6414 gcc_assert (!virtual_operand_p (name)); 6415 6416 tree *loc = vars_map->get (name); 6417 6418 if (!loc) 6419 { 6420 tree decl = SSA_NAME_VAR (name); 6421 if (decl) 6422 { 6423 gcc_assert (!SSA_NAME_IS_DEFAULT_DEF (name)); 6424 replace_by_duplicate_decl (&decl, vars_map, to_context); 6425 /* If name is a default def, then we don't move the defining stmt 6426 (which is a nop). Because (1) the nop doesn't belong to the sese 6427 region, and (2) while setting the def stmt of name to NULL would 6428 trigger release_ssa_name in release_dangling_ssa_names, it wouldn't 6429 be released since it's a default def, and subsequently cause an 6430 ssa verification failure. */ 6431 new_name = make_ssa_name_fn (DECL_STRUCT_FUNCTION (to_context), 6432 decl, SSA_NAME_DEF_STMT (name)); 6433 if (SSA_NAME_IS_DEFAULT_DEF (name)) 6434 set_ssa_default_def (DECL_STRUCT_FUNCTION (to_context), 6435 decl, new_name); 6436 } 6437 else 6438 new_name = copy_ssa_name_fn (DECL_STRUCT_FUNCTION (to_context), 6439 name, SSA_NAME_DEF_STMT (name)); 6440 6441 /* Now that we've used the def stmt to define new_name, make sure it 6442 doesn't define name anymore. */ 6443 SSA_NAME_DEF_STMT (name) = NULL; 6444 6445 vars_map->put (name, new_name); 6446 6447 if (!SSA_NAME_IS_DEFAULT_DEF (name)) 6448 /* The statement has been moved to the child function. It no longer 6449 defines name in the original function. Mark the def stmt NULL, and 6450 let release_dangling_ssa_names deal with it. */ 6451 SSA_NAME_DEF_STMT (name) = NULL; 6452 } 6453 else 6454 new_name = *loc; 6455 6456 return new_name; 6457 } [...] As I said, this is in my WIP tree to merge from trunk into gomp-4_0-branch. To rule out any obvious things, would you please backport to gomp-4_0-branch your trunk commit r227103 (883f001d2c3672e0674bec71f36a2052734a72cf, "Don't create superfluous parm in expand_omp_taskreg"), and revert the following changes as appropriate: > > 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
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. --- 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