This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Time profiler - phase 1


> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index fca665b..3b62bcc 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,31 @@
> +2013-10-29  Martin Liska  <marxin.liska@gmail.com>
> +						Jan Hubicka  <jh@suse.cz>
> +
> +	* cgraph.c (dump_cgraph_node): Profile dump added.
> +	* cgraph.h (struct cgraph_node): New time profile variable added.
> +	* cgraphclones.c (cgraph_clone_node): Time profile is cloned.
> +	* gcov-io.h (gcov_type): New profiler type introduced.
> +	* ipa-profile.c (lto_output_node): Streaming for time profile added.
> +	(input_node): Time profiler is read from LTO stream.
> +	* predict.c (maybe_hot_count_p): Hot prediction changed.
> +	* profile.c (instrument_values): New case for time profiler added.
> +	(compute_value_histograms): Read of time profile.
> +	* tree-pretty-print.c (dump_function_header): Time profiler is dumped.
> +	* tree-profile.c (init_ic_make_global_vars): Time profiler function added.
> +	(gimple_init_edge_profiler): TP function instrumentation.
> +	(gimple_gen_time_profiler): New.
> +	* value-prof.c (gimple_add_histogram_value): Support for time profiler
> +	added.
> +	(dump_histogram_value): TP type added to dumps.
> +	(visit_hist): More sensitive check that takes TP into account.
> +	(gimple_find_values_to_profile): TP instrumentation.
> +	* value-prof.h (hist_type): New histogram type added.
> +	(struct histogram_value_t): Pointer to struct function added.
> +	* libgcc/Makefile.in: New GCOV merge function for TP added.
> +	* libgcov.c: function_counter variable introduced.
> +	(_gcov_merge_time_profile): New.
> +	(_gcov_time_profiler): New.	
> +
>  2013-10-29  David Malcolm  <dmalcolm@redhat.com>
>  
>  	* doc/gty.texi ("Inheritance and GTY"): Make it clear that
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 52d9ab0..c95a54e 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -1890,6 +1890,7 @@ dump_cgraph_node (FILE *f, struct cgraph_node *node)
>    if (node->profile_id)
>      fprintf (f, "  Profile id: %i\n",
>  	     node->profile_id);
> +  fprintf (f, "  First run: %i\n", node->tp_first_run);
>    fprintf (f, "  Function flags:");
>    if (node->count)
>      fprintf (f, " executed "HOST_WIDEST_INT_PRINT_DEC"x",
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index 7706419..479d49f 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -247,7 +247,6 @@ struct GTY(()) cgraph_clone_info
>    bitmap combined_args_to_skip;
>  };
>  
> -
>  /* The cgraph data structure.
>     Each function decl has assigned cgraph_node listing callees and callers.  */
>  
> @@ -324,6 +323,8 @@ struct GTY(()) cgraph_node {
>    unsigned tm_clone : 1;
>    /* True if this decl is a dispatcher for function versions.  */
>    unsigned dispatcher_function : 1;
> +  /* Time profiler: first run of function.  */
> +  int tp_first_run;

Move this up after profile_id.
> --- a/gcc/gcov-io.c
> +++ b/gcc/gcov-io.c
> @@ -68,7 +68,7 @@ gcov_open (const char *name, int mode)
>  #if IN_LIBGCOV
>    const int mode = 0;
>  #endif
> -#if GCOV_LOCKED
> +#if GCOV_LOCKED  
>    struct flock s_flock;
>    int fd;
>  
Accidental change?
> @@ -651,6 +658,9 @@ lto_symtab_prevailing_decl (tree decl)
>    if (TREE_CODE (decl) == FUNCTION_DECL && DECL_BUILT_IN (decl))
>      return decl;
>  
> +  if (!DECL_ASSEMBLER_NAME_SET_P (decl))
> +    return decl;
> +
>    /* Ensure DECL_ASSEMBLER_NAME will not set assembler name.  */
>    gcc_assert (DECL_ASSEMBLER_NAME_SET_P (decl));
>  
Remove this change - it is unrelated hack from my old tree.
> diff --git a/gcc/predict.c b/gcc/predict.c
> index cc9a053..4b655d3 100644
> --- a/gcc/predict.c
> +++ b/gcc/predict.c
> @@ -170,7 +170,7 @@ maybe_hot_count_p (struct function *fun, gcov_type count)
>    if (fun && profile_status_for_function (fun) != PROFILE_READ)
>      return true;
>    /* Code executed at most once is not hot.  */
> -  if (profile_info->runs >= count)
> +  if (count <= 1)
>      return false;
>    return (count >= get_hot_bb_threshold ());
>  }
And also this change.
> @@ -895,9 +907,19 @@ compute_value_histograms (histogram_values values, unsigned cfg_checksum,
>        hist->hvalue.counters =  XNEWVEC (gcov_type, hist->n_counters);
>        for (j = 0; j < hist->n_counters; j++)
>          if (aact_count)
> -	  hist->hvalue.counters[j] = aact_count[j];
> -	else
> -	  hist->hvalue.counters[j] = 0;
> +          hist->hvalue.counters[j] = aact_count[j];
> +        else
> +          hist->hvalue.counters[j] = 0;
> +
> +      if (hist->type == HIST_TYPE_TIME_PROFILE)
> +        {
> +          node = cgraph_get_node (hist->fun->decl);
> +      
> +          node->tp_first_run = hist->hvalue.counters[0];
> +
> +          if (dump_file)
> +            fprintf (dump_file, "Read tp_first_run: %d\n", node->tp_first_run);
> +        }
Probably add a comment why you need to annotate counter here.
> diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
> index b2c5411..0fe262c 100644
> --- a/gcc/tree-pretty-print.c
> +++ b/gcc/tree-pretty-print.c
> @@ -3390,7 +3390,9 @@ dump_function_header (FILE *dump_file, tree fdecl, int flags)
>      fprintf (dump_file, ", decl_uid=%d", DECL_UID (fdecl));
>    if (node)
>      {
> -      fprintf (dump_file, ", symbol_order=%d)%s\n\n", node->symbol.order,
> +      fprintf (dump_file, ", tp_first_run=%d, symbol_order=%d)%s\n\n",
> +               node->tp_first_run,
> +               node->symbol.order,
>                 node->frequency == NODE_FREQUENCY_HOT
>                 ? " (hot)"
>                 : node->frequency == NODE_FREQUENCY_UNLIKELY_EXECUTED

I would skip this change for now - we do not want to increase verbosity too much. Pehraps this can
go with -details or so.
>  static void
>  init_ic_make_global_vars (void)
>  {
> -  tree  gcov_type_ptr;
> +  tree gcov_type_ptr;
>  
> -  ptr_void = build_pointer_type (void_type_node);
> +  ptr_void = build_pointer_type (void_type_node); 
Be cureful about accidental whitespace changes.
>  
>    /* Workaround for binutils bug 14342.  Once it is fixed, remove lto path.  */
>    if (flag_lto)
> @@ -102,7 +104,7 @@ init_ic_make_global_vars (void)
>  
>    varpool_finalize_decl (ic_void_ptr_var);
>  
> -  gcov_type_ptr = build_pointer_type (get_gcov_type ());
> +  gcov_type_ptr = build_pointer_type (get_gcov_type ());  
>    /* Workaround for binutils bug 14342.  Once it is fixed, remove lto path.  */
>    if (flag_lto)
>      {
Here too.
> @@ -455,6 +471,20 @@ gimple_gen_ic_func_profiler (void)
>    gsi_insert_before (&gsi, stmt2, GSI_SAME_STMT);
>  }
>  
> +
> +void
> +gimple_gen_time_profiler (unsigned tag, unsigned base,
> +                          gimple_stmt_iterator &gsi)

Add block comment before function.
> @@ -545,7 +575,7 @@ tree_profiling (void)
>  
>        if (! flag_branch_probabilities
>  	  && flag_profile_values)
> -	gimple_gen_ic_func_profiler ();
> +          gimple_gen_ic_func_profiler ();
>  
>        if (flag_branch_probabilities
>  	  && flag_profile_values
Maybe also accidental?
> @@ -692,13 +702,16 @@ gcov_exit (void)
>  
>  	  if (gi_ptr->merge[t_ix])
>  	    {
> -	      if (!cs_prg->runs++)
> +	      int first_run = !cs_prg->runs;
> +
> +		    cs_prg->runs++;
> +	      if (first_run)
>  	        cs_prg->num = cs_tprg->num;
>  	      cs_prg->sum_all += cs_tprg->sum_all;
>  	      if (cs_prg->run_max < cs_tprg->run_max)
>  		cs_prg->run_max = cs_tprg->run_max;
>  	      cs_prg->sum_max += cs_tprg->run_max;
> -              if (cs_prg->runs == 1)
> +              if (first_run)
>                  memcpy (cs_prg->histogram, cs_tprg->histogram,
>                          sizeof (gcov_bucket_type) * GCOV_HISTOGRAM_SIZE);
>                else

This change is probably part of the patch getting fork working right and unrelated to this patch, right?
I would skip it along with the whitespace changes so Rons' patch to break up libgcov have fewer rejects.
> @@ -795,17 +808,23 @@ gcov_exit (void)
>  	  gcov_write_unsigned (gfi_ptr->cfg_checksum);
>  
>  	  ci_ptr = gfi_ptr->ctrs;
> +
>  	  for (t_ix = 0; t_ix < GCOV_COUNTERS; t_ix++)
>  	    {
> -	      if (!gi_ptr->merge[t_ix])
> +	      gcov_merge_fn merge = gi_ptr->merge[t_ix];
> +
> +	      if (!merge)
>  		continue;
>  
>  	      n_counts = ci_ptr->num;
>  	      gcov_write_tag_length (GCOV_TAG_FOR_COUNTER (t_ix),
>  				     GCOV_TAG_COUNTER_LENGTH (n_counts));
> +
>  	      gcov_type *c_ptr = ci_ptr->values;
> +        gcov_type value;

Whitespace looks wrong.
>  	      while (n_counts--)
> -		gcov_write_counter (*c_ptr++);
> +          gcov_write_counter (*c_ptr++);
> +
>  	      ci_ptr++;
>  	    }
>  	  if (buffered)
> @@ -824,6 +843,8 @@ gcov_exit (void)
>  		   "profiling:%s:Error writing\n",
>  		   gi_filename);
>      }
> +
> +  gcov_clear ();
>  }
>  
>  /* Reset all counters to zero.  */

I believe this is also unrelated (part of fork/vfork changes)
> @@ -851,6 +872,7 @@ gcov_clear (void)
>  		continue;
>  	      
>  	      memset (ci_ptr->values, 0, sizeof (gcov_type) * ci_ptr->num);
> +
>  	      ci_ptr++;
>  	    }
>  	}
> @@ -912,7 +934,6 @@ __gcov_flush (void)
>    __gthread_mutex_lock (&__gcov_flush_mx);
>  
>    gcov_exit ();
> -  gcov_clear ();
>  
>    __gthread_mutex_unlock (&__gcov_flush_mx);
>  }

Also unrelated.
> @@ -974,6 +995,24 @@ __gcov_merge_ior (gcov_type *counters, unsigned n_counters)
>  }
>  #endif
>  
> +#ifdef L_gcov_merge_time_profile
> +void
> +__gcov_merge_time_profile (gcov_type *counters, unsigned n_counters)
> +{
> +  unsigned int i;
> +  gcov_type value;
> +
> +  for (i = 0; i < n_counters; i++)
> +    {
> +      value = gcov_read_counter ();
> +
> +      if (value && (!counters[i] || value < counters[i]))
> +        counters[i] = value;
> +    }
> +}
> +#endif /* L_gcov_merge_time_profile */
Add comments for this function explaining how merging goes and also for
L_gcov_time_profiler.
> +
> +
>  #ifdef L_gcov_merge_single
>  /* The profile merging function for choosing the most common value.
>     It is given an array COUNTERS of N_COUNTERS old counters and it
> @@ -1171,6 +1210,7 @@ __thread
>  #endif
>  gcov_type * __gcov_indirect_call_counters;
>  
> +
>  /* By default, the C++ compiler will use function addresses in the
>     vtable entries.  Setting TARGET_VTABLE_USES_DESCRIPTORS to nonzero
>     tells the compiler to use function descriptors instead.  The value
> @@ -1202,6 +1242,16 @@ __gcov_indirect_call_profiler_v2 (gcov_type value, void* cur_func)
>  }
>  #endif
>  
> +#ifdef L_gcov_time_profiler
> +
> +void
> +__gcov_time_profiler (gcov_type* counters)
> +{
> +  if (!counters[0])
> +    counters[0] = ++function_counter;
> +}
> +#endif
> +
>  #ifdef L_gcov_average_profiler
>  /* Increase corresponding COUNTER by VALUE.  FIXME: Perhaps we want
>     to saturate up.  */
> @@ -1233,10 +1283,12 @@ __gcov_fork (void)
>  {
>    pid_t pid;
>    extern __gthread_mutex_t __gcov_flush_mx;
> +
>    __gcov_flush ();
>    pid = fork ();
>    if (pid == 0)
>      __GTHREAD_MUTEX_INIT_FUNCTION (&__gcov_flush_mx);
> +
>    return pid;
>  }
>  #endif


Please post updated patch. Basically it looks fine with these changes.
Try to also prepare a testcase (in testsuite/gcc.dg/tree-prof you can
see existing examples how to profile and grep dump files for first run counters).

Honza


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]