From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23388 invoked by alias); 20 Nov 2013 20:17:01 -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 23377 invoked by uid 89); 20 Nov 2013 20:17:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 20 Nov 2013 20:16:59 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rAKKGqQe014908 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 20 Nov 2013 15:16:52 -0500 Received: from [10.3.228.53] (vpn-228-53.phx2.redhat.com [10.3.228.53]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rAKKGoTS001839; Wed, 20 Nov 2013 15:16:51 -0500 Message-ID: <1384978578.11568.191.camel@surprise> Subject: Re: [patch] Privatize gimplify_ctx structure. From: David Malcolm To: Jeff Law Cc: Andrew MacLeod , Richard Biener , Trevor Saunders , GCC Patches Date: Wed, 20 Nov 2013 22:21:00 -0000 In-Reply-To: <528D147C.1090102@redhat.com> References: <528CBAA0.5060801@redhat.com> <20131120141658.GJ892@tucnak.redhat.com> <20131120150622.GA19586@tsaunders-iceball.corp.tor1.mozilla.com> <528CE3BA.7010105@redhat.com> <528CE7AE.4020502@redhat.com> <528D020F.7010502@redhat.com> <528D0B13.90803@redhat.com> <528D147C.1090102@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg02618.txt.bz2 On Wed, 2013-11-20 at 12:58 -0700, Jeff Law wrote: > On 11/20/13 12:18, Andrew MacLeod wrote: > > On 11/20/2013 01:40 PM, Jeff Law wrote: > >> On 11/20/13 09:47, Andrew MacLeod wrote: > >>> * gimplify.h (gimplify_hasher : typed_free_remove, struct > >>> gimplify_ctx): > >>> Move to gimplify.c. > >>> * gimplify.c (gimplify_hasher:typed_free_remove): Relocate here. > >>> (struct gimplify_ctx): Relocate here and add 'malloced' field. > >>> (gimplify_ctxp): Make static. > >>> (ctx_pool, ctx_alloc, ctx_free): Manage a list of struct > >>> gimplify_ctx. > >>> (push_gimplify_context): Add default parameters and allocate a > >>> struct from the pool. > >>> (pop_gimplify_context): Free a struct back to the pool. > >>> (gimplify_scan_omp_clauses, gimplify_omp_parallel, > >>> gimplify_omp_task, > >>> gimplify_omp_workshare, gimplify_transaction, gimplify_body): Don't > >>> use a local 'struct gimplify_ctx'. > >>> * gimplify-me.c (force_gimple_operand_1, > >>> gimple_regimplify_operands): > >>> Likewise. > >>> * omp-low.c (lower_omp_sections, lower_omp_single, lower_omp_master, > >>> lower_omp_ordered, lower_omp_critical, lower_omp_for, > >>> create_task_copyfn, lower_omp_taskreg, lower_omp_target, > >>> lower_omp_teams, execute_lower_omp): Likewise. > >>> * gimple-fold.c (gimplify_and_update_call_from_tree): Likewise. > >>> * tree-inline.c (optimize_inline_calls): Likewise. > >> I don't see the malloced field in gimplify_ctx. ChangeLog from prior > >> version? > >> > >> Any reason not to use xcalloc to allocate & clear the memory in > >> ctx_alloc. Oh, I see, you want to clear the cached one too. Nevermind. > >> > >> Should we ever release the list of ctx pointers? > > > > There isn't much of a place to do it... I guess you could export a > > free_gimplify_stack () routine and call at some point at the end of > > finalize_compilation_unit() or something if we think its an issue. Maybe > > the end of cgraphunit.c::expand_all_functions would be the best place... > > it frees its own vector of nodes there as well, and by that point we > > ought to be done. Or is there a better place? > > > > So something like: > > > > Index: cgraphunit.c > > =================================================================== > > *** cgraphunit.c (revision 205035) > > --- cgraphunit.c (working copy) > > *************** expand_all_functions (void) > > *** 1866,1871 **** > > --- 1866,1872 ---- > > } > > } > > cgraph_process_new_functions (); > > + free_gimplify_stack (); > > > > free (order); > > > > > > and > > > > *** gimplify.c 2013-11-20 14:12:57.803369359 -0500 > > --- G.c 2013-11-20 14:15:32.023013391 -0500 > > *************** ctx_free (struct gimplify_ctx *c) > > *** 195,200 **** > > --- 195,215 ---- > > ctx_pool = c; > > } > > > > + /* Free allocated ctx stack memory. */ > > + > > + void > > + free_gimplify_stack (void) > > + { > > + struct gimplify_ctx *c; > > + > > + while (c = ctx_pool) > > + { > > + ctx_pool = c->prev_context; > > + free (c); > > + } > > + } > > + > > + > > > > > > So should we do that? > I think so. Otherwise we just end up adding to the memory leak noise > from valgrind :-0 This kind of thing is a real issue to the JIT library, of course. But having a cleanup routine fixes that. (it's also implicit global state, but that's another battle, I guess; the JIT has a mutex for that).