Re: [PATCH 4/5] State cleanups -- also note for MPX work

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2014-10-16 at 16:06 -0600, Jeff Law wrote:
> On 10/13/14 11:45, David Malcolm wrote:
> > This patch contains various cleanups needed by the jit, so that we can
> > rerun the compiler code repeatedly within the same process:
> >
> > * Introduce per-file finalization routines to clean up file-static
> >    variables.
> >
> > * Make various initializations idempotent (init_ggc), or cleanup their
> >    previous state (init_stringpool).
> >
> > * Introduce a "class toplev", so that we can easy control whether
> >    timevars are reset each time, or run in a "cumulative" mode.
> >
> > gcc/ChangeLog:
> > 	* cgraph.c (cgraph_c_finalize): New function.
> > 	* cgraph.h (cgraph_c_finalize): New prototype.
> > 	(cgraphunit_c_finalize): New prototype.
> > 	(ipa_cp_c_finalize): New prototype.
> > 	* cgraphunit.c (first_analyzed): Move from analyze_functions
> > 	to file-scope.
> > 	(first_analyzed_var): Likewise.
> > 	(analyze_functions): Move static variables into file-scope.
> > 	(cgraphunit_c_finalize): New function.
> > 	* diagnostic.c (diagnostic_finish): Free the memory for
> > 	context->classify_diagnostic and context->printer, running the
> > 	destructor for the latter.
> > 	(bt_stop): Use toplev::main.
> > 	* dwarf2out.c (dwarf2out_finalize): New function.
> > 	* dwarf2out.h (dwarf2out_c_finalize): New prototype.
> > 	* gcse.c (gcse_c_finalize): New function.
> > 	* gcse.h (gcse_c_finalize): New prototype.
> > 	* ggc-page.c (init_ggc): Make idempotent.
> > 	* input.c (input_location): Initialize to UNKNOWN_LOCATION.
> > 	* ipa-cp.c (ipa_cp_c_finalize): New function.
> > 	* ipa-pure-const.c (function_insertion_hook_holder): Move to be
> > 	a field of class pass_ipa_pure_const.
> > 	(node_duplication_hook_holder): Likewise.
> > 	(node_removal_hook_holder): Likewise.
> > 	(register_hooks): Convert to method...
> > 	(pass_ipa_pure_const::register_hooks): ...here, converting
> > 	static variable init_p into...
> > 	(pass_ipa_pure_const::init_p): ...new field.
> > 	(pure_const_generate_summary): Update invocation of
> > 	register_hooks to invoke as a method of current_pass.
> > 	(pure_const_read_summary): Likewise.
> > 	(propagate): Convert to...
> > 	(pass_ipa_pure_const::execute): ...method.
> > 	* ipa-reference.c (ipa_init): Move static bool init_p from here
> > 	to...
> > 	(ipa_init_p): New file-scope variable, so that it can be reset
> > 	when repeatedly invoking the compiler within one process by...
> > 	(ipa_reference_c_finalize): New function.
> > 	* ipa-reference.h (ipa_reference_c_finalize): New.
> > 	* main.c (main): Replace invocation of toplev_main with
> > 	construction of a toplev instance, and call its "main" method.
> > 	* params.c (global_init_params): Add an assert that params_finished is
> > 	false.
> > 	(params_c_finalize): New.
> > 	* params.h (params_c_finalize): New.
> > 	* passes.c (execute_ipa_summary_passes): Set "current_pass" before
> > 	invoking generate_summary, for the benefit of pass_ipa_pure_const.
> > 	(ipa_write_summaries_2): Assign "pass" to "current_pass" global
> > 	before calling write_summary hook.
> > 	(ipa_write_optimization_summaries_1): Likewise when calling
> > 	write_optimization_summary hook.
> > 	(ipa_read_summaries_1): Likewise for read_summary hook.
> > 	(ipa_read_optimization_summaries_1): Likewise for
> > 	read_optimization_summary hook.
> > 	(execute_ipa_stmt_fixups): Likewise.
> > 	* stringpool.c (init_stringpool): Clean up if we're called more
> > 	than once.
> > 	* timevar.c (timevar_init): Ignore repeated calls.
> > 	* toplev.c: Include "dwarf2out.h", "ipa-reference.h", "gcse.h".
> > 	(general_init): Reset "input_location" to UNKNOWN_LOCATION.
> > 	(initialize_rtl): Move static local "initialized_once"
> > 	into file scope, and rename to...
> > 	(rtl_initialized): New variable.
> > 	(do_compile): Move timevar initialization from here to
> > 	toplev::start_timevars.
> > 	(toplev::toplev, toplev::~toplev, toplev::start_timevars,
> > 	toplev::finalize): New functions.
> > 	(toplev_main): Rename to...
> > 	(toplev::main): ...this.
> > 	* toplev.h (class toplev): New class.
> > ---
> >   gcc/cgraph.c         |  14 +++++++
> >   gcc/cgraph.h         |   6 +++
> >   gcc/cgraphunit.c     |  20 ++++++++-
> >   gcc/diagnostic.c     |  11 ++++-
> >   gcc/dwarf2out.c      |  87 +++++++++++++++++++++++++++++++++++++++
> >   gcc/dwarf2out.h      |   2 +
> >   gcc/gcse.c           |   9 ++++
> >   gcc/gcse.h           |   2 +
> >   gcc/ggc-page.c       |   5 +++
> >   gcc/input.c          |   2 +-
> >   gcc/ipa-cp.c         |  12 ++++++
> >   gcc/ipa-pure-const.c | 113 +++++++++++++++++++++++++++++----------------------
> >   gcc/ipa-reference.c  |  17 ++++++--
> >   gcc/ipa-reference.h  |   1 +
> >   gcc/main.c           |   6 ++-
> >   gcc/params.c         |  14 +++++++
> >   gcc/params.h         |   4 ++
> >   gcc/passes.c         |   6 +++
> >   gcc/stringpool.c     |   5 +++
> >   gcc/timevar.c        |   3 ++
> >   gcc/toplev.c         |  67 +++++++++++++++++++++++-------
> >   gcc/toplev.h         |  19 ++++++++-
> >   22 files changed, 351 insertions(+), 74 deletions(-)
> General note, I suspect there's going to be file scoped statics in some 
> of the MPX work.  It was hard to say "you have to do this a different 
> way" and make it a precondition for acceptance with your patch still in 
> progress.
> 
> So that means if the MPX work goes in first, you'll need to adjust this 
> patch.  If it happens the other way, the MPX work will need adjustment.
> 
> Anyway, just thought it was worth explicitly pointing out that these two 
> hunks of work, while they're tackling totally different issues may 
> conflict because of an implementation of the MPX bits.

Presumably my state cleanup patch isn't going to break the MPX bits
though?  I haven't been following them in detail.

I should spell out that I haven't exhaustively hunted down every
file-scoped static in the source tree; this work merely fixes enough
such state to ensure that the testsuite passes: every test is run 5
times in-process, with the equivalent of -O3 -g, both individually, and
within a combined context in test-combination.c, to try to shake out
state issues.  There's also a multithreaded variant of
test-combination.c, test-threads.c, though that's mostly just to ensure
that the big mutex is working and in the right place (it runs every test
case in a separate thread all within one process, each thread running
its case 5 times in a row).

I've got some ideas for automatically tracking down additional
file-scoped static state by analysis of source or of the .o files.  Am I
right in assuming I can regard that as followup bugfixing?


> > diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> > index 20b5c4e..794403d 100644
> > --- a/gcc/cgraph.h
> > +++ b/gcc/cgraph.h
> > @@ -2042,6 +2043,8 @@ bool resolution_used_from_other_file_p (enum ld_plugin_symbol_resolution);
> >   extern bool gimple_check_call_matching_types (gimple, tree, bool);
> >
> >   /* In cgraphunit.c  */
> > +void cgraphunit_c_finalize (void);
> Combine this with the declaration of init_lowered_empty_function so that 
> the references to cgraphunit's functions are together.

I believe they are, since it's immediately followed by...
> > +
> >   /*  Initialize datastructures so DECL is a function in lowered gimple form.
> >       IN_SSA is true if the gimple is in SSA.  */
> >   basic_block init_lowered_empty_function (tree, bool);

^^^ it's here.

Am I missing something?

> > @@ -2061,6 +2064,9 @@ void record_references_in_initializer (tree, bool);
> >   void cgraph_build_static_cdtor (char which, tree body, int priority);
> >   void ipa_discover_readonly_nonaddressable_vars (void);
> >
> > +/* In ipa-cp.c  */
> > +void ipa_cp_c_finalize (void);
> Is there a better place for this?  ipa-prop.h?

OK.

> > diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> > index 59c05ed..a085e0a 100644
> > --- a/gcc/dwarf2out.c
> > +++ b/gcc/dwarf2out.c
> > @@ -24614,4 +24614,91 @@ dwarf2out_finish (const char *filename)
> >       output_indirect_strings ();
> >   }
> >
> > +/* Reset all state within dwarf2out.c so that we can rerun the compiler
> > +   within the same process.  For use by toplev::finalize.  */
> > +
> > +void
> > +dwarf2out_c_finalize (void)
> > +{
> > +  last_var_location_insn = NULL;
> [ ... ]
> Makes me wonder if some of this stuff belongs in a structure.  And if 
> some does, then obviously memset becomes an option for finalization. 
> But that's not something I think you need to own for this to go forward.

FWIW I've been experimenting with making the debugging structs be
classes, with the hooks becoming virtual functions.  Then all of this
debugging state could be turned into member data of the singleton
instance of the appropriate debugging class, created when the
gcc::context is built in toplev initialization.

That would be a much cleaner approach (assuming vfuncs would be
acceptable for the debug hooks) - though I don't yet have a working
patch for this.

So for now I went with the less elegant but equally effective approach.

> > diff --git a/gcc/passes.c b/gcc/passes.c
> > index 5001c3d..772993d 100644
> > --- a/gcc/passes.c
> > +++ b/gcc/passes.c
> > @@ -1944,6 +1944,7 @@ execute_ipa_summary_passes (ipa_opt_pass_d *ipa_pass)
> >   	  if (pass->tv_id)
> >   	    timevar_push (pass->tv_id);
> >
> > +	  current_pass = pass;
> >   	  ipa_pass->generate_summary ();
> Presumably it doesn't make sense to push this down into 
> ipa_pass->generate_summary?

The issue here is that ipa-pure-const.c has some state that ought to be
member data of the pass_ipa_pure_const instance.  The patch makes that
change, the issue is that there's no way to get at the relevant pass
instance just from the IPA pass hooks.  They're not vfuncs, they're just
tables of function pointers.  

(I skipped this part of the conversion of passes to C++ classes since
there wasn't a good way to handle the equivalent of checking to see if a
function ptr is NULL when turning it into a vfunc).

The easy fix is to simply ensure that "current_pass" is set  before
jumping through the IPA hook, so that pass_ipa_pure_const's
implementation can find its instance and get at the data.

> OK with the nits above addressed.

Thanks for the review.

Dave