On Sun, Oct 6, 2013 at 6:51 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> The second part ensures that frequencies are correctly updated after
>> inlining. The problem is that after inlining the frequencies were
>> being recomputed directly from the corresponding bb counts in
>> rebuild_frequencies. If the counts had been truncated to 0, then the
>> recomputed frequencies would become 0 as well (often leading to
>> inconsistencies in the frequencies between blocks). Then the above
>> change to probably_never_executed would not help identify these blocks
>> as non-cold from the frequencies. The fix was to use the existing
>> logic used during static profile rebuilding to also
>> recompute/propagate the frequencies from the branch probabilities in
>> the profile feedback case. I also renamed a number of estimate_*
>> routines to compute_* and updated the comments to reflect the fact
>> that these routines are not doing estimation (from a static profile),
>> but in fact recomputing/propagating frequencies from the existing
>> (either guessed or profile-feedback-based) probabilities.
>
> I see where your plan - you want frequencies to be computed purely on
> probabilities that are there. This however will lead to unnecesary loss of
> precisions for functions with large counts: probabilities are truncated in
> precision and the propagation won't give correct answers and has serious
> truncating issues.
>
> So perhaps we want to execute this only if maximal count in function is low?
> (i.e. less than REG_BR_PROB_BASE or even REG_BR_PROB_BASE/10 - if counts
> are close enough the roundoff errors of count updating should be less terrible
> than errors introduced by the propagation).
Yes, I think that is a better approach.
>
>> -/* Estimate probabilities of loopback edges in loops at same nest level. */
>> +/* Compute frequencies in loops at same nest level. */
>>
>> static void
>> -estimate_loops_at_level (struct loop *first_loop)
>> +compute_loops_at_level (struct loop *first_loop)
>
> I would keep "estimate". The whole logic is not safe and it will lead to
> mistakes for irreducible loops and many other cases...
Ok
>
>> @@ -3027,18 +3042,16 @@ void
>> rebuild_frequencies (void)
>> {
>> timevar_push (TV_REBUILD_FREQUENCIES);
>> - if (profile_status == PROFILE_GUESSED)
>> + if (profile_status == PROFILE_GUESSED || profile_status == PROFILE_READ)
>> {
>> loop_optimizer_init (0);
>> add_noreturn_fake_exit_edges ();
>> mark_irreducible_loops ();
>> connect_infinite_loops_to_exit ();
>> - estimate_bb_frequencies ();
>> + compute_bb_frequencies (true);
>
> Here please add the check about maximal count in functiona and commnet
> (especially for case we will remember to remove this hack if we switch to
> sreal
> based counts/frequencies/probabilities)
>
> OK, with those changes (if it fixes the problem)
Ok thanks.
Teresa
>
> Thanks,
> Honza
--
Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
