Re: [PATCH] decide edge's hotness when there is profile info

[Teresa Johnson <tejohnson at google dot com>]

On Tue, Oct 15, 2013 at 2:57 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Tue, Oct 15, 2013 at 2:03 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> Yes, that would work. So let's discard this patch because the fix for
>> >> comdat can also fix this problem.
>> >
>> > Unforutnately ipa-profile-estimate is an IPA pass and as such it does
>> > not have access to profile_status_to_function.
>>
>> I see. I was coding that up today but hadn't tested it yet.
>>
>> > You can probably just factor this out into a function that can be called
>> > and for normal FDO we call it where the loop stis now and for auto-FDO we can
>> > probably have another invocation from before early passes where auto-FDO is collected.
>>
>> Ok, let's go with that approach for now. It won't address the 0 count
>> COMDAT calling another 0 count COMDAT problem, but I will probably
>> just find a way to deal with this when inlining.
>
> You can still propagate, since tree-profile is an simple-ipa pass.

Ok, I think I will leave that for the second patch, since I need to do
some testing on the effects - i.e. the propagation I had in mind would
make any 0-count routine called by a routine that was dropped to
guessed to also be dropped to guessed (and no longer unlikely). It may
be too aggressive, I need to check.

>>
>> >> >>> +      if (node->count)
>> >> >>> + continue;
>> > Also here we should sum the counts and consider function non unlikely executed
>> > in the same way as probably_never_executed does.
>>
>> I assume you mean by doing the same comparison to the number of
>> profile->runs. Yes, this makes sense.
>
> Yes.
>>
>> >
>> > I can prepare updated patch, but i am currently travelling, so i would not
>> > be disapointed if you beat me ;)
>>
>> I'm working on it, and I think based on Dehao's needs I am going to
>> split up the patch into two phases, the one that does just the part
>> you had sent a patch for (making sure 0 count routines with non-zero
>> calls are marked guessed and have their node frequency set
>> appropriately), and a subsequent one to do the count application when
>> we inline a 0-count routine into a non-zero callsite. I'll shoot for
>> getting this ready by tomorrow.
>>
>> BTW, in your original patch you are checking for both e->count or
>> cgraph_maybe_hot_edge_p(e), but AFAICT the call to
>> cgraph_maybe_hot_edge_p will never return true when e->count is zero.
>> When there is a profile it will return false via maybe_hot_count_p
>> since e->count == 0. When there is no profile it will return false
>> when the callee has NODE_FREQUENCY_UNLIKELY_EXECUTED. So I think just
>> checking for e->count >0 is sufficient here.
>
> I think I was checking caller count here (that is read) and the code
> was supposed to make functoin with hot caller to be hot...

The code in your patch was just checking the edge count, not the
caller count. cgraph_maybe_hot_edge_p also doesn't check the caller
count, just the edge count. Do we want to make all functions with hot
callers also be hot, even when the edge count is not hot? This may be
to aggressive. I was simply going to make the code check e->count.

Teresa

>
> Honza
>>
>> Thanks,
>> Teresa
>>
>> >
>> > Honza
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413