From patchwork Thu Sep 13 19:27:54 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dehao Chen X-Patchwork-Id: 183702 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]) by ozlabs.org (Postfix) with SMTP id D0ECB2C0082 for ; Fri, 14 Sep 2012 05:28:17 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1348169299; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: MIME-Version:Received:Received:In-Reply-To:References:Date: Message-ID:Subject:From:To:Cc:Content-Type:Mailing-List: Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:Sender:Delivered-To; bh=581XgKDqgynvBTcf+7zPEDm2iGY=; b=ARXJaXkrKbIuV0BfO7xx/SX6LEtuSmIE44a+rEfl4de/lDtJFueLg+wk4ELa0z NUpi8lWti9ZqRcTvbfnNtGGU48qUF6mptSNV+C6wylv0ZAmRybe1iSrzCRbqrRMq uECpW1RBxSYKAldFzf5x/83+84It5RqsisbWd3yUMg98w= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:X-Google-DKIM-Signature:Received:MIME-Version:Received:Received:In-Reply-To:References:Date:Message-ID:Subject:From:To:Cc:Content-Type:X-System-Of-Record:X-Gm-Message-State:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=rHXW0/UwG0KZOr9DSI5/kVhSk3aqfyBofVEIrOBFlKt7Qw5mTjK3KSsyECkyx1 fw/ApmrCugHhbXorHHHKhg7UnD/ProU+Qi5Cs9e1jM3Ht5mmVlugbRkA3u5K+bbN aGnLNgh7Z/lAfGqfwGB0CwG8lg1z5sQRrIGGaHsGi6K90=; Received: (qmail 19410 invoked by alias); 13 Sep 2012 19:28:13 -0000 Received: (qmail 19395 invoked by uid 22791); 13 Sep 2012 19:28:10 -0000 X-SWARE-Spam-Status: No, hits=-5.8 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, KHOP_RCVD_TRUST, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, RP_MATCHES_RCVD, TW_TM X-Spam-Check-By: sourceware.org Received: from mail-pb0-f47.google.com (HELO mail-pb0-f47.google.com) (209.85.160.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 13 Sep 2012 19:27:55 +0000 Received: by pbcwy7 with SMTP id wy7so4469894pbc.20 for ; Thu, 13 Sep 2012 12:27:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-system-of-record:x-gm-message-state; bh=j4KmrTDQUnW88vLlIVPSLsmcNvBzONX8NhQI8pqE96w=; b=OCQVv492iq/rZSQpdKmiUHqMw1A+lhnyMN5MfzgCzJiwW+qH7ELVLjSx2w8wYFGzHh 2ZwjgpQJDfbZQKPu8EvpnKX3r07uouRHe5qu91eqR7+JQVV+BOc+4ACNJ/dxFpIdMBAb hFffzeSDQdmvbonOp0/NK1tfRT+E8TOb4bOxetIRnV7L7u9CiOrgBjORDceoK3GAg724 zuaIUyMxMrEQblntyHXvRl8TyZPCF8nOWt83T47MpHkqfdgBEnqqncHWGgGjqAThrllI moHCVMj2mZAT/pIkM+snMyRFLwK+ntazMkr69ADEseEwPxRw5a3h+nhoqYJ4BUpUf7PL 634w== Received: by 10.68.218.72 with SMTP id pe8mr972881pbc.33.1347564475201; Thu, 13 Sep 2012 12:27:55 -0700 (PDT) MIME-Version: 1.0 Received: by 10.68.218.72 with SMTP id pe8mr972860pbc.33.1347564475083; Thu, 13 Sep 2012 12:27:55 -0700 (PDT) Received: by 10.68.200.41 with HTTP; Thu, 13 Sep 2012 12:27:54 -0700 (PDT) In-Reply-To: References: <5049D5AD.5010405@google.com> <504A2EE0.8020704@google.com> Date: Fri, 14 Sep 2012 03:27:54 +0800 Message-ID: Subject: Re: [PATCH] Combine location with block using block_locations From: Dehao Chen To: Richard Guenther Cc: Xinliang David Li , Michael Matz , Diego Novillo , Dodji Seketeli , GCC Patches , Jakub Jelinek , Jan Hubicka , Tom Tromey , Jason Merrill , Richard Henderson X-System-Of-Record: true X-Gm-Message-State: ALoCoQlNiNESE6f1haavm/LQyBE9yZCSNv944V5tODq2WbiwZBGtMEe8BYieYV0jmepJNuDcs5ONBbj40CBipHxSB7OJH5suspVyoN8vKGrNzcm1GJzQJbbk58bBQ95v+WT4XUxf8tpi41F1h9Zmd0WeNe8gRY8K+k/hKbEIgBkRyM5JNf6FzQhW1UwQU4sK1V3qfZzzP+rF X-IsSubscribed: yes 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 On Thu, Sep 13, 2012 at 8:00 PM, Richard Guenther wrote: > On Wed, Sep 12, 2012 at 7:20 PM, Dehao Chen wrote: >> There is another bug in the patch (not covered by unittests, >> discovered through spec benchmarks). >> >> When we remove unused locals, we do not mark the block as used for >> debug stmt, but gimple-streamer-out will still stream out blocks for >> debug stmt. There can be 2 fixes: > > Because doing so would create code generation differences -g vs. -g0. > >> 1. >> --- a/gcc/gimple-streamer-out.c >> +++ b/gcc/gimple-streamer-out.c >> @@ -77,7 +77,8 @@ output_gimple_stmt (struct output_block *ob, gimple stmt) >> lto_output_location (ob, LOCATION_LOCUS (gimple_location (stmt))); >> >> /* Emit the lexical block holding STMT. */ >> - stream_write_tree (ob, gimple_block (stmt), true); >> + if (!is_gimple_debug (stmt)) >> + stream_write_tree (ob, gimple_block (stmt), true); >> >> /* Emit the operands. */ >> switch (gimple_code (stmt)) >> >> 2. >> --- a/gcc/tree-ssa-live.c >> +++ b/gcc/tree-ssa-live.c >> @@ -726,9 +726,6 @@ remove_unused_locals (void) >> gimple stmt = gsi_stmt (gsi); >> tree b = gimple_block (stmt); >> >> - if (is_gimple_debug (stmt)) >> - continue; >> - >> if (gimple_clobber_p (stmt)) >> { >> have_local_clobbers = true; >> >> Either fix could work. Any suggests which one should we go? > > The 2nd one will not work and is not acceptable. The 1st one - well ... > what happens on trunk right now? The debug stmt points to a > BLOCK that is possibly removed from the BLOCK tree? In this case > I think the fix is 3. make sure remove_unused_scope_block_p will > clear BLOCKs from all stmts / expressions that have been removed. > Thanks, updated the patch for this issue. Only attached the diff here, will send out the whole patch with updated ChangeLog later. Bootstrapped and passed all gcc regression tests. And also passed SPEC2006 with LTO. Dehao > Richard. > >> Thanks, >> Dehao >> >> On Wed, Sep 12, 2012 at 10:05 AM, Dehao Chen wrote: >>> There are two parts that needs memory management: >>> >>> 1. The BLOCK structure. This is managed by GC. I originally thought >>> that removing blocks from tree.gsbase would paralyze GC. This turned >>> out not to be a concern because DECL_INITIAL will still mark those >>> used tree nodes. This patch may decrease the memory consumption by >>> removing blocks from tree/gimple. However, as it makes more blocks >>> become used, they also increase the memory consumption. >>> 2. The data structure in libcpp that maintains the hashtable for the >>> location->block mapping. This is relatively minor because for the >>> largest source I've seen, it only maintains less than 100K entries in >>> the array (less than 1M total memory consumption). However, as it is a >>> global data structure, it may make LTO unhappy. Honza is helping >>> testing the memory consumption on LTO (but we first need to make this >>> patch work for LTO). If the LTO result turns out ok, we probably don't >>> want to put these under GC because: 1. it'll make things much more >>> complicated. 2. using self managed memory is more efficient (as this >>> is frequently used in many passes). 3. not using GC actually saves >>> memory because even though the block is in the map, it can still be >>> GCed as soon as it's not reachable from DECL_INITIAL. >>> >>> I've tested this on some very large C++ files (each one takes more >>> than 10s to build), the memory consumption does not see noticeable >>> increase/decrease. >>> >>> Thanks, >>> Dehao >>> >>> On Wed, Sep 12, 2012 at 9:39 AM, Xinliang David Li wrote: >>>> On Wed, Sep 12, 2012 at 2:13 AM, Richard Guenther >>>> wrote: >>>>> On Wed, Sep 12, 2012 at 7:06 AM, Dehao Chen wrote: >>>>>> Now I think we are facing a more complex problem. The data structure >>>>>> we use to store the location_adhoc_data are file-static in linemap.c >>>>>> in libcpp. These data structures are not guarded by GTY(()). >>>>>> Meanwhile, as we have removed the block data structure from >>>>>> gimple.gsbase as well as tree.exp (encoding them into an location_t). >>>>>> This could cause block being GCed and the LOCATION_BLOCK becoming >>>>>> dangling pointers. >>>>> >>>>> Uh. Note that it is quite important that we are able to garbage-collect unused >>>>> BLOCKs, this is the whole point of removing unused BLOCK scopes in >>>>> remove_unused_locals. So this indeed becomes much more complicated ... >>>>> What would be desired is that the garbage collector can NULL an entry in >>>>> the mapping table when it is not referenced in any other way (that other >>>>> reference would be the BLOCK tree as stored in a FUNCTION_DECLs DECL_INITIAL). >>>> >>>> It would be nice to GC those unused BLOCKS. I wonder how many BLOCKS >>>> are created for a large C++ program. This patch saves memory by >>>> shrinking tree size, is it a net win or loss without GC those BLOCKS? >>>> >>>> thanks, >>>> >>>> David >>>> >>>> >>>>> >>>>>> I tried to manipulate GTY to make it recognize the LOCATION_BLOCK from >>>>>> gimple.gsbase.location. However, neigher nested_ptr nor mark_hook can >>>>>> help me. >>>>>> >>>>>> Another approach would be guard the location_adhoc_data and related >>>>>> data structures in GTY(()). However, this is non-trivial because tree >>>>>> is not visible in libcpp. At the same time, my implementation heavily >>>>>> relies on hashtable to make the code efficient, thus it's quite tricky >>>>>> to make "param_is" and "use_params" work. >>>>>> >>>>>> The final approach, which I'll try tomorrow, would be move all my >>>>>> implementation from libcpp to gcc, and guard them with GTY(()). I >>>>>> still haven't thought of any potential problem of this approach. Any >>>>>> comments? >>>>> >>>>> I think moving the mapping to GC in a lazy manner as I described above >>>>> would be the way to go. For hashtables GC already supports if_marked, >>>>> not sure if similar support is available for arrays/vecs. >>>>> >>>>> Richard. >>>>> >>>>>> Thanks, >>>>>> Dehao >>>>>> >>>>>> On Tue, Sep 11, 2012 at 9:00 AM, Dehao Chen wrote: >>>>>>> I saw comments in tree-streamer-out.c: >>>>>>> >>>>>>> /* Do not stream BLOCK_SOURCE_LOCATION. We cannot handle debug information >>>>>>> for early inlining so drop it on the floor instead of ICEing in >>>>>>> dwarf2out.c. */ >>>>>>> streamer_write_chain (ob, BLOCK_VARS (expr), ref_p); >>>>>>> >>>>>>> However, what the code is doing seemed contradictory with the comment. >>>>>>> Or am I missing something? >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, Sep 11, 2012 at 8:32 AM, Michael Matz wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Tue, 11 Sep 2012, Dehao Chen wrote: >>>>>>>> >>>>>>>>> Looks like we have two choices: >>>>>>>>> >>>>>>>>> 1. Stream out block info, and use LTO_SET_PREVAIL for TREE_CHAIN(t) >>>>>>>> >>>>>>>> This will actually not work correctly in some cases. The problem is, if >>>>>>>> the prevailing decl is already part of another chain (say in another >>>>>>>> block_var list) you would break the current chain. Hence block vars need >>>>>>>> special handling in the lto streamer (another reason why tree_chain is not >>>>>>>> the most clever think to use for this chain). This problem area needs to >>>>>>>> be solved somehow if block info is to be preserved correctly. >>>>>>>> >>>>>>>>> 2. Don't stream out block info for LTO, and still call LTO_NO_PREVAIL >>>>>>>>> (TREE_CHAIN (t)). >>>>>>>> >>>>>>>> That's also a large hammer as it basically will mean no debug info after >>>>>>>> LTO :-/ Sigh, at this point I have no good solution that doesn't involve >>>>>>>> quite some work, perhaps your hack is good enough for the time being, >>>>>>>> though I hate it :) >>>>>>> >>>>>>> I got it. Then I'll keep the patch as it is (remove the >>>>>>> LTO_NO_PREVAIL), and work with Honza to resolve the issue he had, and >>>>>>> then we should be good to check in? >>>>>>> >>>>>>> Thanks, >>>>>>> Dehao >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Ciao, >>>>>>>> Michael. diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c index 1381693..af09806 100644 --- a/gcc/tree-ssa-live.c +++ b/gcc/tree-ssa-live.c @@ -612,6 +612,47 @@ mark_all_vars_used (tree *expr_p) walk_tree (expr_p, mark_all_vars_used_1, NULL, NULL); } +/* Helper function for clear_unused_block_pointer, called via walk_tree. */ + +static tree +clear_unused_block_pointer_1 (tree *tp, int *, void *) +{ + if (EXPR_P (*tp) && TREE_BLOCK (*tp) + && !TREE_USED (TREE_BLOCK (*tp))) + TREE_SET_BLOCK (*tp, NULL); + if (TREE_CODE (*tp) == VAR_DECL && DECL_DEBUG_EXPR_IS_FROM (*tp)) + { + tree debug_expr = DECL_DEBUG_EXPR (*tp); + walk_tree (&debug_expr, clear_unused_block_pointer_1, NULL, NULL); + } + return NULL_TREE; +} + +/* Set all block pointer in debug stmt to NULL if the block is unused, + so that they will not be streamed out. */ + +static void +clear_unused_block_pointer () +{ + basic_block bb; + gimple_stmt_iterator gsi; + FOR_EACH_BB (bb) + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) + { + unsigned i; + tree b; + gimple stmt = gsi_stmt (gsi); + + if (!is_gimple_debug (stmt)) + continue; + b = gimple_block (stmt); + if (b && !TREE_USED (b)) + gimple_set_block (stmt, NULL); + for (i = 0; i < gimple_num_ops (stmt); i++) + walk_tree (gimple_op_ptr (stmt, i), clear_unused_block_pointer_1, + NULL, NULL); + } +} /* Dump scope blocks starting at SCOPE to FILE. INDENT is the indentation level and FLAGS is as in print_generic_expr. */ @@ -841,6 +882,7 @@ remove_unused_locals (void) VEC_truncate (tree, cfun->local_decls, dstidx); remove_unused_scope_block_p (DECL_INITIAL (current_function_decl)); + clear_unused_block_pointer (); BITMAP_FREE (usedvars);