Re: [patch] Privatize gimplify_ctx structure.

[David Malcolm <dmalcolm at redhat dot com>]

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).