Re: [PATCH] New fdo summary-based icache sensitive unrolling (issue 6351086)

[Teresa Johnson <tejohnson at google dot com>]

Resending in plain text mode...sigh.

Teresa


On Thu, Jul 26, 2012 at 1:32 PM, Teresa Johnson <tejohnson@google.com> wrote:
>
>
>
>
> On Thu, Jul 26, 2012 at 11:26 AM, <davidxl@google.com> wrote:
>>
>>
>> http://codereview.appspot.com/6351086/diff/1/gcc/gcov-io.h
>> File gcc/gcov-io.h (right):
>>
>> http://codereview.appspot.com/6351086/diff/1/gcc/gcov-io.h#newcode396
>> gcc/gcov-io.h:396: gcov_unsigned_t num_hot_counters;/* number of
>>
>> counters to reach a given
>> Should it be made into an array accessed with pre-defined indices?
>
>
> You mean provide an array of values for different thresholds? In that case both num_hot_counters and hot_cutoff_value would need to be arrays (oh, I see now that you indicate that below too). And we would possibly need another array to hold the thresholds, unless they are hard coded values. I've been thinking about extending this to provide data for more threshold values, but was holding that off for a follow-on, when I had a better idea of how the expanded summary info would be used.
>
> Honza - you were talking about using this type of summary info to guide hot/cold decisions, do you find the current single threshold data useful enough?
>
>>
>> http://codereview.appspot.com/6351086/diff/1/gcc/gcov-io.h#newcode399
>> gcc/gcov-io.h:399: num_hot_counters.  */
>> Similarly for this one -- or put the above two values into a pair and
>> declare an array of the pairs?
>>
>> http://codereview.appspot.com/6351086/diff/1/libgcc/libgcov.c
>> File libgcc/libgcov.c (right):
>>
>> http://codereview.appspot.com/6351086/diff/1/libgcc/libgcov.c#newcode332
>> libgcc/libgcov.c:332: if (cs_ptr->sum_all*cutoff_perc < cs_ptr->sum_all)
>> space needed
>>
>
> I think I have added the space you wanted (around the '*' char) - let me know if you meant something else.
>
>>
>> http://codereview.appspot.com/6351086/diff/1/libgcc/libgcov.c#newcode332
>> libgcc/libgcov.c:332: if (cs_ptr->sum_all*cutoff_perc < cs_ptr->sum_all)
>> space needed. Is it better to check if (cs_ptr->sum_all * cutoff_perc
>> /cutoff_perc != cs_ptr->sum_all)?
>
>
> Good suggestion - changed.
>
>>
>>
>> http://codereview.appspot.com/6351086/diff/1/libgcc/libgcov.c#newcode718
>> libgcc/libgcov.c:718: if (cs_tprg->num_hot_counters >
>> cs_prg->num_hot_counters)
>> Why guard the update under !cs_prg->runs++ --> the condition is used to
>> identify if it is a first write or a merged write.  (verify with a test
>> case to see if the merge of the summary is correct).
>
>
> In fact the ">" check doesn't make sense if this is the first write, which is currently how I have it guarded. The question is how to best "merge" this information. After thinking about this, I am going to move this code out from under the !cs_prg->runs++ check, so that on a merge, we use the summary with the largest num_hot_counters.
>
> I am going to do a little testing with my changes and will upload a new patch.
>
> Thanks,
> Teresa
>
>>
>>
>> http://codereview.appspot.com/6351086/
>
>
>
>
> --
> Teresa Johnson | Software Engineer |  tejohnson@google.com |  408-460-2413
>



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