Fix timevar internal consistency failure

Hi,

On Wed, 10 Feb 2016, Richard Biener wrote:

> > The problem is that TV_PHASE_DBGINFO is now nested within 
> > TV_PHASE_OPT_GEN, which violates the above mutual exclusivity 
> > requirement.  Therefore the attached patch simply gets rid of 
> > TV_PHASE_DBGINFO (as well as of the sibling TV_PHASE_CHECK_DBGINFO 
> > which was already unused).
> >
> > Tested on x86_64-suse-linux, OK for the mainline?
> 
> Ok.

I had this in my tree for a while, asserting that such nesting doesn't 
happen (it asserts that we're always in some phase, and that phases don't 
nest).  Might be a good addition for gcc 7.


Ciao,
Michael.

On Wed, 2016-02-10 at 15:15 +0100, Michael Matz wrote:
> Hi,
> 
> On Wed, 10 Feb 2016, Richard Biener wrote:
> 
> > > The problem is that TV_PHASE_DBGINFO is now nested within 
> > > TV_PHASE_OPT_GEN, which violates the above mutual exclusivity 
> > > requirement.  Therefore the attached patch simply gets rid of 
> > > TV_PHASE_DBGINFO (as well as of the sibling
> > > TV_PHASE_CHECK_DBGINFO 
> > > which was already unused).
> > > 
> > > Tested on x86_64-suse-linux, OK for the mainline?
> > 
> > Ok.
> 
> I had this in my tree for a while, asserting that such nesting
> doesn't 
> happen (it asserts that we're always in some phase, and that phases
> don't 
> nest).  Might be a good addition for gcc 7.

> Ciao,
> Michael.
> 
> Index: timevar.c
> ===================================================================
> --- timevar.c	(revision 232927)
> +++ timevar.c	(working copy)
> @@ -325,6 +325,8 @@ timer::push (timevar_id_t timevar)
>    push_internal (tv);
>  }
>  
> +static timevar_id_t global_phase;

FWIW I like the idea, but could this be a private field within class
timer, rather than a global?  I moved the global state relating to
timevars into a "class timer" in r223092 (the jit uses this, via a
followup: r226530: in theory different threads can have different timer
instances [1]).

I see that all of the accesses are within timer:: methods.  Maybe
"m_current_phase" or somesuch?

[1] https://gcc.gnu.org/onlinedocs/jit/topics/performance.html#the-timi
ng-api
and there's a TV_JIT_ACQUIRING_MUTEX which a thread uses for time spent
waiting to acquire the big jit mutex (which guards e.g. the g_timer
singleton pointer.  Within toplev and below, g_timer is the single
"live" instance of class timer, but there can be multiple threads each
with timer instances waiting to call into toplev via gcc_jit_context_co
mpile, if that makes sense).

>  /* Push TV onto the timing stack, either one of the builtin ones
>     for a timevar_id_t, or one provided by client code to libgccjit. 
>  */
>  
> @@ -350,6 +352,8 @@ timer::push_internal (struct timevar_def
>    if (m_stack)
>      timevar_accumulate (&m_stack->timevar->elapsed, &m_start_time,
> &now);
>  
> +  gcc_assert (global_phase >= TV_PHASE_SETUP
> +	      && global_phase <= TV_PHASE_FINALIZE);
>    /* Reset the start time; from now on, time is attributed to
>       TIMEVAR.  */
>    m_start_time = now;
> @@ -432,6 +436,9 @@ timer::start (timevar_id_t timevar)
>  {
>    struct timevar_def *tv = &m_timevars[timevar];
>  
> +  gcc_assert (global_phase == TV_NONE || global_phase == TV_TOTAL);
> +  global_phase = timevar;
> +
>    /* Mark this timing variable as used.  */
>    tv->used = 1;
>  
> @@ -463,6 +470,12 @@ timer::stop (timevar_id_t timevar)
>    struct timevar_def *tv = &m_timevars[timevar];
>    struct timevar_time_def now;
>  
> +  gcc_assert (global_phase == timevar);
> +  if (timevar == TV_TOTAL)
> +    global_phase = TV_NONE;
> +  else
> +    global_phase = TV_TOTAL;
> +
>    /* TIMEVAR must have been started via timevar_start.  */
>    gcc_assert (tv->standalone);
>    tv->standalone = 0; /* Enable a restart.  */

Hi,

On Wed, 10 Feb 2016, David Malcolm wrote:

> > +static timevar_id_t global_phase;
> 
> FWIW I like the idea, but could this be a private field within class
> timer, rather than a global?

Sure, consider the patch amended accordingly.


Ciao,
Michael.