On Mon, Nov 11, 2013 at 7:23 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> 2013-11-08 Teresa Johnson <tejohn...@google.com>
>> Jan Hubicka <j...@suse.cz>
>>
>> * predict.c (drop_profile): New function.
>> (handle_missing_profiles): Ditto.
>> (counts_to_freqs): Don't overwrite estimated frequencies
>> when function has no profile counts.
>> * predict.h (handle_missing_profiles): Declare.
>> * tree-inline.c (freqs_to_counts): New function.
>> (copy_cfg_body): Invoke freqs_to_counts as needed.
>> * tree-profile.c (tree_profiling): Invoke handle_missing_profiles.
>>
>> Index: predict.c
>> ===================================================================
>> --- predict.c (revision 204516)
>> +++ predict.c (working copy)
>> @@ -2765,6 +2765,107 @@ estimate_loops (void)
>> BITMAP_FREE (tovisit);
>> }
>>
>> +/* Drop the profile for NODE to guessed, and update its frequency based on
>> + whether it is expected to be HOT. */
>> +
>> +static void
>> +drop_profile (struct cgraph_node *node, bool hot)
>> +{
>> + struct function *fn = DECL_STRUCT_FUNCTION (node->decl);
>> +
>> + if (dump_file)
>> + fprintf (dump_file,
>> + "Dropping 0 profile for %s/%i. %s based on calls.\n",
>> + cgraph_node_name (node), node->order,
>> + hot ? "Function is hot" : "Function is normal");
>> + /* 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))
>> + warning (0,
>> + "Missing counts for called function %s/%i",
>> + cgraph_node_name (node), node->order);
>> +
>> + profile_status_for_function (fn)
>> + = (flag_guess_branch_prob ? PROFILE_GUESSED : PROFILE_ABSENT);
>> + node->frequency
>> + = hot ? NODE_FREQUENCY_HOT : NODE_FREQUENCY_NORMAL;
>> +}
>> +
>> +/* In the case of COMDAT routines, multiple object files will contain the
>> same
>> + function and the linker will select one for the binary. In that case
>> + all the other copies from the profile instrument binary will be missing
>> + profile counts. Look for cases where this happened, due to non-zero
>> + call counts going to 0-count functions, and drop the profile to guessed
>> + so that we can use the estimated probabilities and avoid optimizing only
>> + for size.
>> +
>> + The other case where the profile may be missing is when the routine
>> + is not going to be emitted to the object file, e.g. for "extern template"
>> + class methods. Those will be marked DECL_EXTERNAL. Emit a warning in
>> + all other cases of non-zero calls to 0-count functions. */
>> +
>> +void
>> +handle_missing_profiles (void)
>> +{
>> + struct cgraph_node *node;
>> + int unlikely_count_fraction = PARAM_VALUE (UNLIKELY_BB_COUNT_FRACTION);
>> + vec<struct cgraph_node *> worklist;
>> + worklist.create (64);
>> +
>> + /* See if 0 count function has non-0 count callers. In this case we
>> + lost some profile. Drop its function profile to PROFILE_GUESSED. */
>> + FOR_EACH_DEFINED_FUNCTION (node)
>> + {
>> + struct cgraph_edge *e;
>> + gcov_type call_count = 0;
>> + struct function *fn = DECL_STRUCT_FUNCTION (node->decl);
>> +
>> + if (node->count)
>> + continue;
>> + for (e = node->callers; e; e = e->next_caller)
>> + call_count += e->count;
>> + if (call_count
>> + && 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);
>> + worklist.safe_push (node);
>> + }
>
> Perhaps we should add an note/error on mishandled profile when function is
> not COMDAT?
> That way we may notice further bugs in this area.
I have a warning like that already in drop_profile(). Is that
sufficient? Also, Steven Bosscher suggested putting that warning under
OPT_Wdisabled_optimization instead of '0', what do you prefer for
that?
>> + }
>> +
>> + /* Propagate the profile dropping to other 0-count COMDATs that are
>> + potentially called by COMDATs we already dropped the profile on. */
>> + while (worklist.length () > 0)
>> + {
>> + struct cgraph_edge *e;
>> +
>> + node = worklist.pop ();
>> + for (e = node->callees; e; e = e->next_caller)
>> + {
>> + struct cgraph_node *callee = e->callee;
>> + struct function *fn = DECL_STRUCT_FUNCTION (callee->decl);
>> +
>> + if (callee->count > 0)
>> + continue;
>> + if (DECL_COMDAT (callee->decl) && fn && fn->cfg
>> + && profile_status_for_function (fn) == PROFILE_READ)
>
> Perhaps we can check here maybe_hot_bb_p for the caller and rely on profile
> estimate
> to give us false only for really known to be unlikely paths? (i.e. EH
> handling, noreturns
> etc.)
Ok, let me try this.
>> + {
>> + /* 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);
>> + worklist.safe_push (callee);
>> + }
>> + }
>> + }
>> + worklist.release ();
>
> I would add a pointer set so we avoid duplicates in worklist. This is
> potentially quadratic
> for large programs.
I'll add a visited_nodes set to keep track of processed nodes so they
don't get added to the worklist multiple times.
Thanks,
Teresa
>
> OK, with these changes.
>
> Honza
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
