From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29157 invoked by alias); 5 Aug 2015 11:13:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 28353 invoked by uid 89); 5 Aug 2015 11:13:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Wed, 05 Aug 2015 11:13:43 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 96D5FAD85; Wed, 5 Aug 2015 11:13:40 +0000 (UTC) Date: Wed, 05 Aug 2015 11:13:00 -0000 From: Richard Biener To: Tom de Vries cc: Thomas Schwinge , GCC Patches , Jakub Jelinek Subject: Re: [committed, gomp4] Fix release_dangling_ssa_names In-Reply-To: <55C1EA05.5070904@mentor.com> Message-ID: 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> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2015-08/txt/msg00259.txt.bz2 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. 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. > > 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. Thanks, Richard.