Re: [PATCH 1/2] gcc parts of timer refactoring

[Jeff Law <law@redhat.com>]

On 07/31/2015 03:45 PM, David Malcolm wrote:
> In r223092 (aka dd4d567f4b6b498242097c41d63666bdae320ac1) I moved the
> state of timevar.c from being global data into a "class timer" with
> a global instance "g_timer".
>
> This followup patch generalizes the timing within toplev so that an
> external "timer" instance can (optionally) be passed in; this is used
> in patch 2 to add an API to libgccjit for client code: gcc_jit_timer.
>
> The gcc_jit_timer API allows client code to create arbitrary new timing
> items (beyond the hardcoded list in timevar.def), giving them names,
> so that client code can time whatever things are relevant to it; this
> patch adds the support necessary to timevar.[ch] for this.
>
> The patch also generalizes timevar.h's auto_timevar to avoid it
> depending on the "g_timer" global (which is only set during the
> lifetime of "toplev".  We can use this to add TV_JIT_ACQUIRING_MUTEX,
> and thus measure wallclock time spent waiting for the JIT mutex.
>
> Generalize timevar.c to add support for timing jit client code, by:
>
>   * adding support for timing client items with arbitrary
>     client-supplied names (e.g. "compiling code", "running code" etc).
>
>   * supporting having a "timer" instance created externally and passed
>     in to toplev, allowing clients to control where toplev's timing
>     information is accumulated
>
>   * add a couple of jit-specific timevars
>
> This goes with the next patch; I've split them up for ease of review.
>
> OK for trunk?
>
> gcc/ChangeLog:
> 	* main.c (main): Pass in NULL for toplev's external_timer.
> 	* timevar.c: Include coretypes.h, hash-map.h, vec.h.
> 	(class timer::named_items): New.
> 	(timer::named_items::named_items): New.
> 	(timer::named_items::~named_items): New.
> 	(timer::named_items::push): New.
> 	(timer::named_items::pop): New.
> 	(timer::named_items::print): New.
> 	(timer::timer): Initialize field "m_jit_client_items".
> 	(timer::~timer): New.
> 	(timer::push): Move bulk of implementation to...
> 	(timer::push_internal): ...here.  New function.
> 	(timer::pop): Move bulk of implementation to...
> 	(timer::pop_internal): ...here.  New function.
> 	(timer::push_client_item): New.
> 	(timer::pop_client_item): New.
> 	(timer::print_row): New function, taken from timer::print.
> 	(timer::print): Print "GCC items" header if we also have client
> 	items.  Move row-printing to timer::print_row.  Print any client
> 	items.
> 	(timer::get_topmost_item_name): New method.
> 	* timevar.def (TV_JIT_ACQUIRING_MUTEX): New.
> 	(TV_JIT_CLIENT_CODE): New.
> 	* timevar.h (timer::push_client_item): New declaration.
> 	(timer::pop_client_item): New declaration.
> 	(timer::get_topmost_item_name): New method.
> 	(timer::push_internal): New declaration.
> 	(timer::pop_internal): New declaration.
> 	(timer::print_row): New declaration.
> 	(timer::named_items): New declaration.
> 	(timer::m_jit_client_items): New field.
> 	(timer): Add friend class named_items.
> 	(auto_timevar::auto_timevar): Add timer param.
> 	(auto_timevar::~auto_timevar): Use field "m_timer".
> 	(auto_timevar::m_timer): New field.
> 	* toplev.c (initialize_rtl): Add g_timer as param when
> 	constructing auto_timevar instance.
> 	(toplev::toplev): Add "external_timer" param, and use it to
> 	initialize the "g_timer" global if non-NULL.
> 	(toplev::~toplev): If this created "g_timer", delete it.
> 	* toplev.h (toplev::toplev): Replace "use_TV_TOTAL" bool param
> 	with "external_timer" timer *.
> ---
>   gcc/main.c      |   2 +-
>   gcc/timevar.c   | 247 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>   gcc/timevar.def |   2 +
>   gcc/timevar.h   |  35 +++++++-
>   gcc/toplev.c    |  18 +++--
>   gcc/toplev.h    |   4 +-
>   6 files changed, 261 insertions(+), 47 deletions(-)
>
> diff --git a/gcc/main.c b/gcc/main.c
> index 79baf0d..bbd8b67 100644
> --- a/gcc/main.c
> +++ b/gcc/main.c
> @@ -33,7 +33,7 @@ int main (int argc, char **argv);
>   int
>   main (int argc, char **argv)
>   {
> -  toplev toplev (true, /* use_TV_TOTAL */
> +  toplev toplev (NULL, /* external_timer */
>   		 true /* init_signals */);
>
>     return toplev.main (argc, argv);
> diff --git a/gcc/timevar.c b/gcc/timevar.c
> index 76ad22a..eebb1dc 100644
> --- a/gcc/timevar.c
> +++ b/gcc/timevar.c
> @@ -20,7 +20,10 @@ along with GCC; see the file COPYING3.  If not see
>
>   #include "config.h"
>   #include "system.h"
> +#include "coretypes.h"
>   #include "timevar.h"
> +#include "hash-map.h"
> +#include "vec.h"
You shouldn't need to include hash-map.h and vec.h, as those get pulled 
in via core-types.h including hash-table.h now.

> +  for (iter = m_stack; iter; iter=next)
Nit, whitespace around the '=' in iter=next assignment.

> +    {
> +      next = iter->next;
> +      free (iter);
> +    }
> +  for (iter = m_unused_stack_instances; iter; iter=next)
Here too.

OK with those nits fixed.

jeff