decide edge's hotness when there is profile info

On Tue, Nov 12, 2013 at 2:35 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Tue, Nov 12, 2013 at 1:33 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> More info on the lto bootstrap failure:
>> >>
>> >> /usr/local/google/home/tejohnson/gcc_trunk_9/libiberty/pex-unix.c:790:1:
>> >> warning: Missing counts for called function pex_child_error.isra.1/73
>> >> [enabled by default]
>> >>  }
>> >>
>> >> This is an error handling routine that invokes _exit. Looking at the
>> >> ipa-profile dump, I can see that all the counts read in for
>> >> pex_child_error have the value 0. But there is a non-zero callsite,
>> >> that not surprisingly complains of a profile insanity in the bb's
>> >> outgoing edge weights, since pex_child_error never returns:
>> >>
>> >> ;;   basic block 38, loop depth 1, count 192, freq 5000
>> >> ;;   Invalid sum of outgoing counts 0, should be 192
>> >> ...
>> >>   pex_child_error.isra.1D.5005 (_92, executable_40(D),
>> >> [/usr/local/google/home/tejohnson/gcc_trunk_9/libiberty/pex-unix.c :
>> >> 677] "execv", _91);
>> >> ;;    succ:       3 [100.0%]  (ABNORMAL,DFS_BACK,IRREDUCIBLE_LOOP,EXECUTABLE)
>> >>
>> >> Not sure why the counts were all 0 for this routine though, I wouldn't
>> >> think the early exit should prevent us from updating the counts. But
>> >> IMO the best thing to do here is to issue a warning, since I don't
>> >> want more issues like this to cause compile errors when we handled
>> >> them fine before.
>> >>
>> >> Let me know if the patch below is ok.
>> >
>> > OK, so _exit actually makes us to miss streaming of the path leading to the
>> > error message?  Handling of vfork is somewhat broken, as I noticed with Martin
>> > Liska. One problem is that gcov_exit is not clearing the counts.  With vfork
>> > being used in addition to execvs we stream out the data but the counts stays in
>> > the oriignal process memory and then are streamed again.  If execve fails and
>> > leads to _exit I think we can get the miscompare you see.
>> >
>> > Does calling gcov_clear within gcov_exit help?  (It is what I have in our tree)
>>
>> I thought this was already handled by the __gcov_execv wrapper around
>> execv which invokes __gcov_flush that in turns does a gcov_exit
>> followed by gcov_clear?
>>
>> I think the issue is that we register gcov_exit with atexit(), but
>> _exit does not execute any atexit functions by definition. So that
>> would explain why the counters from pex_child_error didn't get dumped.
>> The caller bb invokes execv before invoking pex_child_error:
>>           execv (executable, to_ptr32 (argv));
>>           pex_child_error (obj, executable, "execv", errno);
>> So the counters of the caller bb are dumped (hence the caller bb has
>> non-zero counts) and cleared, and the pex_child_error does not dump
>> its counters since it uses _exit instead of exit and therefore doesn't
>> invoke gcov_exit atexit.
>
> Hmm, I see, the problem here is that execv gets BB splitted (and profile is correct)
> but before we get into ipa_profile the BBs get re-merged and we incorrectly put
> count of 1 to pex_child_error.
>
> I suppose this will always happen when you have function terminating program
> (execve) before other function call.  Perhaps we can warn only when the difference in counts
> is greater than number of train runs?

Ok, that sounds good. Here is the new patch. Is this ok for trunk if
testing (bootstrap regression and lto profiledbootstrap) succeeds?

Thanks,
Teresa

2013-11-13  Teresa Johnson  <tejohnson@google.com>

        * predict.c (drop_profile): Error is currently too strict.
        (handle_missing_profiles): Pass call_count to drop_profile.


>
> Honza

> Ok, that sounds good. Here is the new patch. Is this ok for trunk if
> testing (bootstrap regression and lto profiledbootstrap) succeeds?
> 
> Thanks,
> Teresa
> 
> 2013-11-13  Teresa Johnson  <tejohnson@google.com>
> 
>         * predict.c (drop_profile): Error is currently too strict.
>         (handle_missing_profiles): Pass call_count to drop_profile.

OK, thanks

Honza
> 
> Index: predict.c
> ===================================================================
> --- predict.c   (revision 204704)
> +++ predict.c   (working copy)
> @@ -2766,12 +2766,17 @@ estimate_loops (void)
>  }
> 
>  /* Drop the profile for NODE to guessed, and update its frequency based on
> -   whether it is expected to be HOT.  */
> +   whether it is expected to be hot given the CALL_COUNT.  */
> 
>  static void
> -drop_profile (struct cgraph_node *node, bool hot)
> +drop_profile (struct cgraph_node *node, gcov_type call_count)
>  {
>    struct function *fn = DECL_STRUCT_FUNCTION (node->decl);
> +  /* In the case where this was called by another function with a
> +     dropped profile, call_count will be 0. Since there are no
> +     non-zero call counts to this function, we don't know for sure
> +     whether it is hot, and therefore it will be marked normal below.  */
> +  bool hot = maybe_hot_count_p (NULL, call_count);
> 
>    if (dump_file)
>      fprintf (dump_file,
> @@ -2781,8 +2786,13 @@ static void
>    /* We only expect to miss profiles for functions that are reached
>       via non-zero call edges in cases where the function may have
>       been linked from another module or library (COMDATs and extern
> -     templates). See the comments below for handle_missing_profiles.  */
> -  if (!DECL_COMDAT (node->decl) && !DECL_EXTERNAL (node->decl))
> +     templates). See the comments below for handle_missing_profiles.
> +     Also, only warn in cases where the missing counts exceed the
> +     number of training runs. In certain cases with an execv followed
> +     by a no-return call the profile for the no-return call is not
> +     dumped and there can be a mismatch.  */
> +  if (!DECL_COMDAT (node->decl) && !DECL_EXTERNAL (node->decl)
> +      && call_count > profile_info->runs)
>      {
>        if (flag_profile_correction)
>          {
> @@ -2792,8 +2802,8 @@ static void
>                       cgraph_node_name (node), node->order);
>          }
>        else
> -        error ("Missing counts for called function %s/%i",
> -               cgraph_node_name (node), node->order);
> +        warning (0, "Missing counts for called function %s/%i",
> +                 cgraph_node_name (node), node->order);
>      }
> 
>    profile_status_for_function (fn)
> @@ -2839,9 +2849,7 @@ handle_missing_profiles (void)
>            && fn && fn->cfg
>            && (call_count * unlikely_count_fraction >= profile_info->runs))
>          {
> -          bool maybe_hot = maybe_hot_count_p (NULL, call_count);
> -
> -          drop_profile (node, maybe_hot);
> +          drop_profile (node, call_count);
>            worklist.safe_push (node);
>          }
>      }
> @@ -2863,11 +2871,7 @@ handle_missing_profiles (void)
>            if (DECL_COMDAT (callee->decl) && fn && fn->cfg
>                && profile_status_for_function (fn) == PROFILE_READ)
>              {
> -              /* Since there are no non-0 call counts to this function,
> -                 we don't know for sure whether it is hot. Indicate to
> -                 the drop_profile routine that function should be marked
> -                 normal, rather than hot.  */
> -              drop_profile (node, false);
> +              drop_profile (node, 0);
>                worklist.safe_push (callee);
>              }
>          }
> 
> >
> > Honza
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413