From patchwork Tue Aug 11 18:53:39 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 506204 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 6BA2D140187 for ; Wed, 12 Aug 2015 04:53:59 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=idfpgSyK; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=kZBKJDcR42eHh9/cA sj+fbd3MG68aPCHjxcZqMdltqeSkEkrpLBaOp59hltGItJ/V+/6MdkTl3MzlreT7 pIRYH94Nd2aSUHOV+TiLX9S+KbPLilsQlIE1I9+RjQ46/1+HzVexCRyIJfemLd3w pNdVDPl89X4dPl//NvQnN+fyYY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=oHbyCLFkx4rqSVXovHBw2CO PoTI=; b=idfpgSyKBGIZcyla977mtRI7BcWbhr4QaRa+TA994HWrVkDGMJ96YnU 4iwCOCiTuKOwkZcRwvDCCTSV6tqKOdzjkpYmra1J/CXKdYwCrMpdEGtQ3UxgdfNM /ZHtrMvd1W5NmN4vo0KfhlWLCyYKk/E7fZzXr3xAPFpsvZL//RAA= Received: (qmail 97532 invoked by alias); 11 Aug 2015 18:53:53 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 97242 invoked by uid 89); 11 Aug 2015 18:53:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 11 Aug 2015 18:53:50 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1ZPEgE-0000mT-7I from Tom_deVries@mentor.com ; Tue, 11 Aug 2015 11:53:46 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.3.224.2; Tue, 11 Aug 2015 19:53:44 +0100 Message-ID: <55CA44B3.2000409@mentor.com> Date: Tue, 11 Aug 2015 20:53:39 +0200 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Richard Biener CC: Thomas Schwinge , GCC Patches , Jakub Jelinek Subject: [PATCH] Don't create superfluous parm in expand_omp_taskreg References: <546743BC.5070804@mentor.com> <54678B3D.8020009@mentor.com> <54730EE7.40000@mentor.com> <5474665A.4070101@mentor.com> <87iocp1d8o.fsf@kepler.schwinge.homeip.net> <557076A5.7050207@mentor.com> <55C1BA11.70008@mentor.com> <55C1CDAF.8050402@mentor.com> <55C1EA05.5070904@mentor.com> In-Reply-To: [ 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 * 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 *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 *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 (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 *bbs_p); extern void verify_sese (basic_block, basic_block, vec *); +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