Re: Reorgnanization of profile count maintenance code, part 1

[Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>]

On Thu, Jun 01, 2017 at 01:35:56PM +0200, Jan Hubicka wrote:

Just some very minor nits.

> Index: final.c
> ===================================================================
> --- final.c	(revision 248684)
> +++ final.c	(working copy)
> @@ -1951,9 +1951,11 @@ dump_basic_block_info (FILE *file, rtx_i
>        fprintf (file, "%s BLOCK %d", ASM_COMMENT_START, bb->index);
>        if (bb->frequency)
>          fprintf (file, " freq:%d", bb->frequency);
> -      if (bb->count)
> -        fprintf (file, " count:%" PRId64,
> -                 bb->count);
> +      if (bb->count.initialized_p ())
> +	{
> +          fprintf (file, " count");

Missing colon.
s/count"/count:"/

> Index: profile-count.h
> ===================================================================
> --- profile-count.h	(revision 0)
> +++ profile-count.h	(working copy)

> +/* Main data type to hold profile counters in GCC.  In most cases profile
> +   counts originate from profile feedback. They are 64bit integers
> +   representing number of executions during the train run.
> +   As the profile is maintained during the compilation, many adjustments are
> +   made.  Not all transformations can be made precisely, most importantly
> +   when code is being duplicated.  It also may happen that part of CFG has
> +   profile counts known while other does not - for example when LTO optimizing

s/does not/do not/;# i think

> +   partly profiled program or when profile was lost due to COMDAT merging.
> +
> +   For this information profile_count trakcs more information than

tracks

> +class GTY(()) profile_count
> +{

> +  /* Value of counters which has not been intiailized. Either becuase

initialized
because

> +     initializatoin did not happen yet or because profile is unknown.  */

initialization

> +  static profile_count uninitialized ()
> +    {
> +      profile_count c;
> +      c.m_val = -1;
> +      return c;
> +    }
> +
> +  /* The profiling runtime uses gcov_type, which is usually 64bit integer.
> +     Conversins back and forth are used to read the coverage and get it

Conversions

> +  profile_count &operator+= (const profile_count &other)

I think i saw a_count = a_count + something above and assumed you didn't
have a += operator. Could thus use the terse form in the snipped code
above on the patch, maybe?

> +  /* Return *this * num / den.  */

Parameter names in comment in caps please.

> Index: tree-tailcall.c
> ===================================================================
> --- tree-tailcall.c	(revision 248684)
> +++ tree-tailcall.c	(working copy)
> @@ -767,12 +767,10 @@ adjust_return_value (basic_block bb, tre
>  /* Subtract COUNT and FREQUENCY from the basic block and it's
>     outgoing edge.  */
>  static void
> -decrease_profile (basic_block bb, gcov_type count, int frequency)
> +decrease_profile (basic_block bb, profile_count count, int frequency)
>  {
>    edge e;
> -  bb->count -= count;
> -  if (bb->count < 0)
> -    bb->count = 0;
> +  bb->count = bb->count - count;

That's one of the spots where i'd have expected use of operator -=, fwiw.

> Index: value-prof.c
> ===================================================================
> --- value-prof.c	(revision 248684)
> +++ value-prof.c	(working copy)
> @@ -588,8 +588,10 @@ free_histograms (struct function *fn)
>  
>  static bool
>  check_counter (gimple *stmt, const char * name,
> -	       gcov_type *count, gcov_type *all, gcov_type bb_count)
> +	       gcov_type *count, gcov_type *all, profile_count bb_count_d)
>  {
> +  gcov_type bb_count = bb_count_d.to_gcov_type ();
> +  return true;

On purpose?

thanks,