On Mon, Nov 11, 2013 at 8:22 AM, Jan Hubicka <[email protected]> wrote:
>> I have a warning like that already in drop_profile(). Is that
>
> I think it should be warning (or silent) for COMDAT and error/note for
> other functions (depending on flag_profile_correction).
> I guess drop_profile is better place for it indeed.
I don't want to warn for COMDAT since this is currently an expected
issue in that case, so I'm afraid it will be too noisy (but there is a
note emitted to the dump file to track how often this occurs). So
currently the warning is only emitted in cases where we don't expect
it to occur (e.g. non-comdat).
>
>> sufficient? Also, Steven Bosscher suggested putting that warning under
>> OPT_Wdisabled_optimization instead of '0', what do you prefer for
>> that?
>
> It is a bit different case than other disabled optimizations we have (where
> the optimization does not happen because of --param tunable limits), but I
> am fine with both alternatives.
> The warning may end up quite noisy so having way to silence it definitely
> makes sense.
I didn't find any warnings being emitted when running this for spec or
internal benchmarks, so how about instead of a warning, I handle it
like you suggest above: depending on the setting of
flag_profile_correction either error or emit a note to the dump file
under MSG_MISSED_OPTIMIZATION?
>>
>> >> + }
>> >> +
>> >> + /* 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.
>
> Perhaps you can also track this by testing profile_status_for_function. Both
> solutions are fine for me.
Ah - I just realized I am already checking profile_status_for_function
above and adding to the worklist if it is still PROFILE_READ. Since I
call drop_profile before adding to the worklist, we can't end up
adding multiple times. So I don't think there is currently an issue
with this.
Thanks,
Teresa
>
> Honza
>>
>> Thanks,
>> Teresa
>>
>> >
>> > OK, with these changes.
>> >
>> > Honza
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | [email protected] | 408-460-2413
--
Teresa Johnson | Software Engineer | [email protected] | 408-460-2413
