From: Tom de Vries <Tom_deVries@mentor.com>
To: Richard Biener <rguenther@suse.de>
Cc: Thomas Schwinge <thomas@codesourcery.com>,
GCC Patches <gcc-patches@gcc.gnu.org>,
Jakub Jelinek <jakub@redhat.com>
Subject: Re: [committed, gomp4] Fix release_dangling_ssa_names
Date: Wed, 05 Aug 2015 10:49:00 -0000 [thread overview]
Message-ID: <55C1EA05.5070904@mentor.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1508051116450.19642@zhemvz.fhfr.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
next prev parent reply other threads:[~2015-08-05 10:49 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-15 14:08 openacc kernels directive -- initial support Tom de Vries
2014-11-15 17:21 ` [PATCH, 1/8] Expand oacc kernels after pass_build_ealias Tom de Vries
2014-11-24 11:29 ` Tom de Vries
2014-11-25 11:30 ` Tom de Vries
2015-04-21 19:40 ` Expand oacc kernels after pass_fre (was: [PATCH, 1/8] Expand oacc kernels after pass_build_ealias) Thomas Schwinge
2015-04-22 7:36 ` Richard Biener
2015-06-04 16:50 ` Expand oacc kernels after pass_fre Tom de Vries
2015-06-08 7:29 ` Richard Biener
2015-06-19 9:04 ` Tom de Vries
2015-08-05 7:24 ` [committed, gomp4] Fix release_dangling_ssa_names Tom de Vries
2015-08-05 7:29 ` Richard Biener
2015-08-05 8:48 ` Tom de Vries
2015-08-05 9:30 ` Richard Biener
2015-08-05 10:49 ` Tom de Vries [this message]
2015-08-05 11:13 ` Richard Biener
2015-08-11 9:25 ` [committed] Add todo comment for move_sese_region_to_fn Tom de Vries
2015-08-11 18:53 ` [PATCH] Don't create superfluous parm in expand_omp_taskreg Tom de Vries
2015-08-12 10:51 ` Richard Biener
2015-09-24 6:36 ` Thomas Schwinge
2015-09-24 7:21 ` Tom de Vries
2015-09-24 9:31 ` Thomas Schwinge
2015-09-30 8:05 ` [gomp4,committed] Remove release_dangling_ssa_names Tom de Vries
2015-09-30 10:05 ` Thomas Schwinge
2015-09-30 10:25 ` Tom de Vries
2015-09-30 10:43 ` Thomas Schwinge
2014-11-15 17:22 ` [PATCH, 2/8] Add pass_oacc_kernels Tom de Vries
2014-11-25 11:31 ` Tom de Vries
2015-04-21 19:46 ` Thomas Schwinge
2014-11-15 17:23 ` [PATCH, 4/8] Add pass_tree_loop_{init,done} to pass_oacc_kernels Tom de Vries
2014-11-25 11:42 ` Tom de Vries
2015-04-21 19:52 ` Thomas Schwinge
2015-04-22 7:40 ` Richard Biener
2015-06-02 13:52 ` Tom de Vries
2015-06-02 13:58 ` Richard Biener
2015-06-02 15:40 ` Tom de Vries
2015-06-03 11:26 ` Richard Biener
2014-11-15 17:23 ` [PATCH, 3/8] Add pass_ch_oacc_kernels " Tom de Vries
2014-11-25 11:39 ` Tom de Vries
2015-04-21 19:49 ` Thomas Schwinge
2015-04-22 7:39 ` Richard Biener
2015-06-03 9:22 ` Tom de Vries
2015-06-03 11:21 ` Richard Biener
2015-06-04 15:59 ` Tom de Vries
2015-06-03 10:05 ` Tom de Vries
2015-06-03 11:22 ` Richard Biener
2014-11-15 17:24 ` [PATCH, 5/8] Add pass_loop_im " Tom de Vries
2014-11-25 12:00 ` Tom de Vries
2015-04-21 19:57 ` [PATCH, 5/8] Add pass_lim " Thomas Schwinge
2014-11-15 18:32 ` [PATCH, 6/8] Add pass_ccp " Tom de Vries
2014-11-25 12:03 ` Tom de Vries
2015-04-21 20:01 ` [PATCH, 6/8] Add pass_copy_prop in pass_oacc_kernels Thomas Schwinge
2015-04-22 7:42 ` Richard Biener
2015-06-02 13:04 ` Tom de Vries
2014-11-15 18:52 ` [PATCH, 7/8] Add pass_parloops_oacc_kernels to pass_oacc_kernels Tom de Vries
2014-11-25 12:15 ` Tom de Vries
2015-04-21 20:09 ` [PATCH, 7/8] Add pass_parallelize_loops_oacc_kernels " Thomas Schwinge
2014-11-15 19:04 ` [PATCH, 8/8] Do simple omp lowering for no address taken var Tom de Vries
2014-11-17 10:29 ` Richard Biener
2014-11-18 9:13 ` Eric Botcazou
2014-11-18 9:53 ` Richard Biener
2014-11-18 12:20 ` Richard Biener
2014-11-24 11:53 ` Tom de Vries
2014-11-24 11:55 ` Tom de Vries
2014-11-24 12:42 ` Richard Biener
2014-11-24 18:49 ` Tom de Vries
2014-11-24 12:40 ` Richard Biener
2014-11-19 20:34 ` openacc kernels directive -- initial support Tom de Vries
2015-04-21 19:27 ` Add BUILT_IN_GOACC_KERNELS_INTERNAL (was: openacc kernels directive -- initial support) Thomas Schwinge
2015-04-21 20:24 ` Handle global loop counters in fortran oacc kernels " Thomas Schwinge
2015-04-21 20:29 ` Handle global loop counters in c/c++ " Thomas Schwinge
2015-04-21 20:33 ` Handle oacc kernels with other directives " Thomas Schwinge
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55C1EA05.5070904@mentor.com \
--to=tom_devries@mentor.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=rguenther@suse.de \
--cc=thomas@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).