Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[davidxl]

http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h
File gcc/gcov-io.h (right):

http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode544
gcc/gcov-io.h:544: gcov_unsigned_t sum_cutoff_percent;/* sum_all cutoff
percentage computed
Is there a need to record this?

http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode546
gcc/gcov-io.h:546: gcov_unsigned_t num_to_cutoff;/* number of counters
to reach above cutoff.  */
This should have a better name -- e.g., hot_arc_count.

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c
File gcc/loop-unroll.c (right):

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode196
gcc/loop-unroll.c:196: or peeled loop.  */
Add more documentation here: 1) for program size < threshold, do not
limit 2) for    threshold < psize < 2* threshold, tame the max allows
peels/unrolls according to hotness; 3) for huge footprint programs,
disable it (by ...).

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode197
gcc/loop-unroll.c:197: static limit_type
Blank line.

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode233
gcc/loop-unroll.c:233: gcc_assert(profile_info->sum_all > 0);
Do not assert on profile data -- either bail or emit info.

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode237
gcc/loop-unroll.c:237: if (profile_info->num_to_cutoff <
size_threshold*2) {
space

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode238
gcc/loop-unroll.c:238: /* For appliations that are less than twice the
codesize limit, allow
applications

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode532
gcc/loop-unroll.c:532: limit = limit_code_size(loop, &codesize_divisor);
space.

http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode1095
gcc/loop-unroll.c:1095: int codesize_divisor)
This parameter is not documented. The name of the parameter is also not
ideal -. Is it possible to not change the interfaces? -- i.e., split
limit_code_size into two helper functions one to get the the suppress
flag as before, and the other gets the limit_factor which is called
inside each function 'decide_unroll_runtime...' -- it is cleaner and
easier to understand that way.

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c
File libgcc/libgcov.c (right):

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode832
libgcc/libgcov.c:832: #define CUM_CUTOFF_PERCENT_TIMES_10 999
Ok now but it should be controllable at compile time with a --param --
recorded in the binary;

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode839
libgcc/libgcov.c:839: for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE;
t_ix++)
There does not seem a need for a loop -- only t_ix == GCOV_COUNTER_ARCS
is summable.

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode848
libgcc/libgcov.c:848: cum_cutoff = (cs_ptr->sum_all * cutoff_perc)/1000;
Overflow possibility?

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode854
libgcc/libgcov.c:854: value_array = (gcov_type *) malloc
(sizeof(gcov_type)*cs_ptr->num);
space

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode860
libgcc/libgcov.c:860: for (i = 0, ctr_info_ix = 0; i < t_ix; i++)
No need for this -- the index for ARC counter is always 0 (add assert)
-- so either skip (use merge function for 47 and mask for 46) or use 0.

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode864
libgcc/libgcov.c:864: }
in gcc_46 and before, the counters may not be allocated, and it should
not
directly accessed using t_ix. It needs to be guarded with

if ((1 << t_ix) & gi_ptr->ctr_mask)

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode875
libgcc/libgcov.c:875: gcc_assert (index + ci_ptr->num <= cs_ptr->num);
Need to relax this  a little by skipping -- profiling dumping is known
to be 'flaky'

http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode1103
libgcc/libgcov.c:1103: cs_prg->sum_max += cs_tprg->run_max;
For multiple runs, how should num_to_cutoff merged? Pick the larger
value?

http://codereview.appspot.com/6282045/

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Teresa Johnson]

On Tue, Jun 5, 2012 at 11:46 AM,  <davidxl@google.com> wrote:
>
> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h
> File gcc/gcov-io.h (right):
>
> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode544
> gcc/gcov-io.h:544: gcov_unsigned_t sum_cutoff_percent;/* sum_all cutoff
> percentage computed
> Is there a need to record this?

It isn't used by by the profile-use compile, I thought it might be
useful to have in knowing how to interpret the count. But I can remove
it.

>
> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode546
> gcc/gcov-io.h:546: gcov_unsigned_t num_to_cutoff;/* number of counters
>
> to reach above cutoff.  */
> This should have a better name -- e.g., hot_arc_count.

Ok. I have changed it to "num_hot_counters" to be less specific to the
type of counter the the summary represents.

>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c
> File gcc/loop-unroll.c (right):
>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode196
> gcc/loop-unroll.c:196: or peeled loop.  */
> Add more documentation here: 1) for program size < threshold, do not
> limit 2) for    threshold < psize < 2* threshold, tame the max allows
> peels/unrolls according to hotness; 3) for huge footprint programs,
> disable it (by ...).

done.

>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode197
> gcc/loop-unroll.c:197: static limit_type
> Blank line.

fixed

>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode233
> gcc/loop-unroll.c:233: gcc_assert(profile_info->sum_all > 0);
> Do not assert on profile data -- either bail or emit info.

done.

>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode237
> gcc/loop-unroll.c:237: if (profile_info->num_to_cutoff <
> size_threshold*2) {
> space

where? i added space around the "*" and also fixed up the curly brace
formatting in this block.

>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode238
> gcc/loop-unroll.c:238: /* For appliations that are less than twice the
> codesize limit, allow
> applications

fixed

>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode532
> gcc/loop-unroll.c:532: limit = limit_code_size(loop, &codesize_divisor);
> space.

fixed (at new callsites, see below)

>
> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode1095
> gcc/loop-unroll.c:1095: int codesize_divisor)
> This parameter is not documented. The name of the parameter is also not
> ideal -. Is it possible to not change the interfaces? -- i.e., split
> limit_code_size into two helper functions one to get the the suppress
> flag as before, and the other gets the limit_factor which is called
> inside each function 'decide_unroll_runtime...' -- it is cleaner and
> easier to understand that way.

Yes, in fact I have restructured it so that it is only called within
the decide_unroll and decide_peel
routines.

>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c
> File libgcc/libgcov.c (right):
>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode832
> libgcc/libgcov.c:832: #define CUM_CUTOFF_PERCENT_TIMES_10 999
> Ok now but it should be controllable at compile time with a --param --
> recorded in the binary;

ok

>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode839
> libgcc/libgcov.c:839: for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE;
> t_ix++)
> There does not seem a need for a loop -- only t_ix == GCOV_COUNTER_ARCS
> is summable.

i wanted to write the code generically, so it would be forward
compatible and match the rest of the code.

>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode848
> libgcc/libgcov.c:848: cum_cutoff = (cs_ptr->sum_all * cutoff_perc)/1000;
> Overflow possibility?

Good point. I changed this to do the divide first and then the
multiply. Won't have any noticeable effect given the large threshold
used by the consumer of this info.

>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode854
> libgcc/libgcov.c:854: value_array = (gcov_type *) malloc
> (sizeof(gcov_type)*cs_ptr->num);
> space

done

>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode860
> libgcc/libgcov.c:860: for (i = 0, ctr_info_ix = 0; i < t_ix; i++)
> No need for this -- the index for ARC counter is always 0 (add assert)

I thought about using that to simplify this code, but was trying to be
as generic as possible for forward compatibility in case anything
changed. What do you think?

> -- so either skip (use merge function for 47 and mask for 46) or use 0.
>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode864
> libgcc/libgcov.c:864: }
> in gcc_46 and before, the counters may not be allocated, and it should
> not
> directly accessed using t_ix. It needs to be guarded with
>
> if ((1 << t_ix) & gi_ptr->ctr_mask)

The code for 4_6 is simpler and looks different because the counters
are structured differently.

>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode875
> libgcc/libgcov.c:875: gcc_assert (index + ci_ptr->num <= cs_ptr->num);
> Need to relax this  a little by skipping -- profiling dumping is known
> to be 'flaky'

fixed, now handle this case gracefully.

>
> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode1103
> libgcc/libgcov.c:1103: cs_prg->sum_max += cs_tprg->run_max;
> For multiple runs, how should num_to_cutoff merged? Pick the larger
> value?

that's a good idea - fixed.

New patch coming shortly.

Thanks,
Teresa

>
> http://codereview.appspot.com/6282045/



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

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Xinliang David Li]

The patch looks like 4_7 based which can be different from 4_6 based
one. Do you have the latter one for review?

David

On Wed, Jun 6, 2012 at 2:02 PM, Teresa Johnson <tejohnson@google.com> wrote:
> On Tue, Jun 5, 2012 at 11:46 AM,  <davidxl@google.com> wrote:
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h
>> File gcc/gcov-io.h (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode544
>> gcc/gcov-io.h:544: gcov_unsigned_t sum_cutoff_percent;/* sum_all cutoff
>> percentage computed
>> Is there a need to record this?
>
> It isn't used by by the profile-use compile, I thought it might be
> useful to have in knowing how to interpret the count. But I can remove
> it.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode546
>> gcc/gcov-io.h:546: gcov_unsigned_t num_to_cutoff;/* number of counters
>>
>> to reach above cutoff.  */
>> This should have a better name -- e.g., hot_arc_count.
>
> Ok. I have changed it to "num_hot_counters" to be less specific to the
> type of counter the the summary represents.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c
>> File gcc/loop-unroll.c (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode196
>> gcc/loop-unroll.c:196: or peeled loop.  */
>> Add more documentation here: 1) for program size < threshold, do not
>> limit 2) for    threshold < psize < 2* threshold, tame the max allows
>> peels/unrolls according to hotness; 3) for huge footprint programs,
>> disable it (by ...).
>
> done.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode197
>> gcc/loop-unroll.c:197: static limit_type
>> Blank line.
>
> fixed
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode233
>> gcc/loop-unroll.c:233: gcc_assert(profile_info->sum_all > 0);
>> Do not assert on profile data -- either bail or emit info.
>
> done.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode237
>> gcc/loop-unroll.c:237: if (profile_info->num_to_cutoff <
>> size_threshold*2) {
>> space
>
> where? i added space around the "*" and also fixed up the curly brace
> formatting in this block.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode238
>> gcc/loop-unroll.c:238: /* For appliations that are less than twice the
>> codesize limit, allow
>> applications
>
> fixed
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode532
>> gcc/loop-unroll.c:532: limit = limit_code_size(loop, &codesize_divisor);
>> space.
>
> fixed (at new callsites, see below)
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode1095
>> gcc/loop-unroll.c:1095: int codesize_divisor)
>> This parameter is not documented. The name of the parameter is also not
>> ideal -. Is it possible to not change the interfaces? -- i.e., split
>> limit_code_size into two helper functions one to get the the suppress
>> flag as before, and the other gets the limit_factor which is called
>> inside each function 'decide_unroll_runtime...' -- it is cleaner and
>> easier to understand that way.
>
> Yes, in fact I have restructured it so that it is only called within
> the decide_unroll and decide_peel
> routines.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c
>> File libgcc/libgcov.c (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode832
>> libgcc/libgcov.c:832: #define CUM_CUTOFF_PERCENT_TIMES_10 999
>> Ok now but it should be controllable at compile time with a --param --
>> recorded in the binary;
>
> ok
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode839
>> libgcc/libgcov.c:839: for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE;
>> t_ix++)
>> There does not seem a need for a loop -- only t_ix == GCOV_COUNTER_ARCS
>> is summable.
>
> i wanted to write the code generically, so it would be forward
> compatible and match the rest of the code.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode848
>> libgcc/libgcov.c:848: cum_cutoff = (cs_ptr->sum_all * cutoff_perc)/1000;
>> Overflow possibility?
>
> Good point. I changed this to do the divide first and then the
> multiply. Won't have any noticeable effect given the large threshold
> used by the consumer of this info.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode854
>> libgcc/libgcov.c:854: value_array = (gcov_type *) malloc
>> (sizeof(gcov_type)*cs_ptr->num);
>> space
>
> done
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode860
>> libgcc/libgcov.c:860: for (i = 0, ctr_info_ix = 0; i < t_ix; i++)
>> No need for this -- the index for ARC counter is always 0 (add assert)
>
> I thought about using that to simplify this code, but was trying to be
> as generic as possible for forward compatibility in case anything
> changed. What do you think?
>
>> -- so either skip (use merge function for 47 and mask for 46) or use 0.
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode864
>> libgcc/libgcov.c:864: }
>> in gcc_46 and before, the counters may not be allocated, and it should
>> not
>> directly accessed using t_ix. It needs to be guarded with
>>
>> if ((1 << t_ix) & gi_ptr->ctr_mask)
>
> The code for 4_6 is simpler and looks different because the counters
> are structured differently.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode875
>> libgcc/libgcov.c:875: gcc_assert (index + ci_ptr->num <= cs_ptr->num);
>> Need to relax this  a little by skipping -- profiling dumping is known
>> to be 'flaky'
>
> fixed, now handle this case gracefully.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode1103
>> libgcc/libgcov.c:1103: cs_prg->sum_max += cs_tprg->run_max;
>> For multiple runs, how should num_to_cutoff merged? Pick the larger
>> value?
>
> that's a good idea - fixed.
>
> New patch coming shortly.
>
> Thanks,
> Teresa
>
>>
>> http://codereview.appspot.com/6282045/
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Xinliang David Li]

On Wed, Jun 6, 2012 at 2:02 PM, Teresa Johnson <tejohnson@google.com> wrote:
> On Tue, Jun 5, 2012 at 11:46 AM,  <davidxl@google.com> wrote:
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h
>> File gcc/gcov-io.h (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode544
>> gcc/gcov-io.h:544: gcov_unsigned_t sum_cutoff_percent;/* sum_all cutoff
>> percentage computed
>> Is there a need to record this?
>
> It isn't used by by the profile-use compile, I thought it might be
> useful to have in knowing how to interpret the count. But I can remove
> it.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode546
>> gcc/gcov-io.h:546: gcov_unsigned_t num_to_cutoff;/* number of counters
>>
>> to reach above cutoff.  */
>> This should have a better name -- e.g., hot_arc_count.
>
> Ok. I have changed it to "num_hot_counters" to be less specific to the
> type of counter the the summary represents.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c
>> File gcc/loop-unroll.c (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode196
>> gcc/loop-unroll.c:196: or peeled loop.  */
>> Add more documentation here: 1) for program size < threshold, do not
>> limit 2) for    threshold < psize < 2* threshold, tame the max allows
>> peels/unrolls according to hotness; 3) for huge footprint programs,
>> disable it (by ...).
>
> done.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode197
>> gcc/loop-unroll.c:197: static limit_type
>> Blank line.
>
> fixed
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode233
>> gcc/loop-unroll.c:233: gcc_assert(profile_info->sum_all > 0);
>> Do not assert on profile data -- either bail or emit info.
>
> done.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode237
>> gcc/loop-unroll.c:237: if (profile_info->num_to_cutoff <
>> size_threshold*2) {
>> space
>
> where? i added space around the "*" and also fixed up the curly brace
> formatting in this block.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode238
>> gcc/loop-unroll.c:238: /* For appliations that are less than twice the
>> codesize limit, allow
>> applications
>
> fixed
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode532
>> gcc/loop-unroll.c:532: limit = limit_code_size(loop, &codesize_divisor);
>> space.
>
> fixed (at new callsites, see below)
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode1095
>> gcc/loop-unroll.c:1095: int codesize_divisor)
>> This parameter is not documented. The name of the parameter is also not
>> ideal -. Is it possible to not change the interfaces? -- i.e., split
>> limit_code_size into two helper functions one to get the the suppress
>> flag as before, and the other gets the limit_factor which is called
>> inside each function 'decide_unroll_runtime...' -- it is cleaner and
>> easier to understand that way.
>
> Yes, in fact I have restructured it so that it is only called within
> the decide_unroll and decide_peel
> routines.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c
>> File libgcc/libgcov.c (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode832
>> libgcc/libgcov.c:832: #define CUM_CUTOFF_PERCENT_TIMES_10 999
>> Ok now but it should be controllable at compile time with a --param --
>> recorded in the binary;
>
> ok
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode839
>> libgcc/libgcov.c:839: for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE;
>> t_ix++)
>> There does not seem a need for a loop -- only t_ix == GCOV_COUNTER_ARCS
>> is summable.
>
> i wanted to write the code generically, so it would be forward
> compatible and match the rest of the code.


Here it is different -- if there were multiple summable counters,
adding/sorting them together may not make sense. It is the arc counter
that needs to be collected.

>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode848
>> libgcc/libgcov.c:848: cum_cutoff = (cs_ptr->sum_all * cutoff_perc)/1000;
>> Overflow possibility?
>
> Good point. I changed this to do the divide first and then the
> multiply. Won't have any noticeable effect given the large threshold
> used by the consumer of this info.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode854
>> libgcc/libgcov.c:854: value_array = (gcov_type *) malloc
>> (sizeof(gcov_type)*cs_ptr->num);
>> space
>
> done
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode860
>> libgcc/libgcov.c:860: for (i = 0, ctr_info_ix = 0; i < t_ix; i++)
>> No need for this -- the index for ARC counter is always 0 (add assert)
>
> I thought about using that to simplify this code, but was trying to be
> as generic as possible for forward compatibility in case anything
> changed. What do you think?

It is fine to keep this with t_ix == arc_counter -- there is one minor
bug -- before the loop, the following should be added to skip all the
functions:

if (!gi_ptr->merge[t_ix])
  continue;


David

>
>> -- so either skip (use merge function for 47 and mask for 46) or use 0.
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode864
>> libgcc/libgcov.c:864: }
>> in gcc_46 and before, the counters may not be allocated, and it should
>> not
>> directly accessed using t_ix. It needs to be guarded with
>>
>> if ((1 << t_ix) & gi_ptr->ctr_mask)
>
> The code for 4_6 is simpler and looks different because the counters
> are structured differently.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode875
>> libgcc/libgcov.c:875: gcc_assert (index + ci_ptr->num <= cs_ptr->num);
>> Need to relax this  a little by skipping -- profiling dumping is known
>> to be 'flaky'
>
> fixed, now handle this case gracefully.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode1103
>> libgcc/libgcov.c:1103: cs_prg->sum_max += cs_tprg->run_max;
>> For multiple runs, how should num_to_cutoff merged? Pick the larger
>> value?
>
> that's a good idea - fixed.
>
> New patch coming shortly.
>
> Thanks,
> Teresa
>
>>
>> http://codereview.appspot.com/6282045/
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Jan Hubicka]

> The patch looks like 4_7 based which can be different from 4_6 based
> one. Do you have the latter one for review?

Hi,
the code determining where program spend most of time in more sane way than
current hot/cold decision logic is long time on my TODO. My plan was to count
number of gimple statments executed at link time in ipa-profile pass and then
scale the logic based on it. But siply sorting gcov counts ignoring the fact
htat we have no clue how many instruction given counter controls don't seem
like bad logic. (especially for non-LTO compilations).
Any plans fo rmainline version?
Honza

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Xinliang David Li]

On Wed, Jun 6, 2012 at 2:02 PM, Teresa Johnson <tejohnson@google.com> wrote:
> On Tue, Jun 5, 2012 at 11:46 AM,  <davidxl@google.com> wrote:
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h
>> File gcc/gcov-io.h (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode544
>> gcc/gcov-io.h:544: gcov_unsigned_t sum_cutoff_percent;/* sum_all cutoff
>> percentage computed
>> Is there a need to record this?
>
> It isn't used by by the profile-use compile, I thought it might be
> useful to have in knowing how to interpret the count. But I can remove
> it.
>

It is ok to remove it. Suggested by Honza, I think it is also very
useful to store the cutoff count value (or a set of values e.g at 10,
20, ... 90% ) -- gcc currently rely too much on the
hot_bb_count_fraction to make decision .

thanks,

David

>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode546
>> gcc/gcov-io.h:546: gcov_unsigned_t num_to_cutoff;/* number of counters
>>
>> to reach above cutoff.  */
>> This should have a better name -- e.g., hot_arc_count.
>
> Ok. I have changed it to "num_hot_counters" to be less specific to the
> type of counter the the summary represents.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c
>> File gcc/loop-unroll.c (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode196
>> gcc/loop-unroll.c:196: or peeled loop.  */
>> Add more documentation here: 1) for program size < threshold, do not
>> limit 2) for    threshold < psize < 2* threshold, tame the max allows
>> peels/unrolls according to hotness; 3) for huge footprint programs,
>> disable it (by ...).
>
> done.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode197
>> gcc/loop-unroll.c:197: static limit_type
>> Blank line.
>
> fixed
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode233
>> gcc/loop-unroll.c:233: gcc_assert(profile_info->sum_all > 0);
>> Do not assert on profile data -- either bail or emit info.
>
> done.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode237
>> gcc/loop-unroll.c:237: if (profile_info->num_to_cutoff <
>> size_threshold*2) {
>> space
>
> where? i added space around the "*" and also fixed up the curly brace
> formatting in this block.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode238
>> gcc/loop-unroll.c:238: /* For appliations that are less than twice the
>> codesize limit, allow
>> applications
>
> fixed
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode532
>> gcc/loop-unroll.c:532: limit = limit_code_size(loop, &codesize_divisor);
>> space.
>
> fixed (at new callsites, see below)
>
>>
>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode1095
>> gcc/loop-unroll.c:1095: int codesize_divisor)
>> This parameter is not documented. The name of the parameter is also not
>> ideal -. Is it possible to not change the interfaces? -- i.e., split
>> limit_code_size into two helper functions one to get the the suppress
>> flag as before, and the other gets the limit_factor which is called
>> inside each function 'decide_unroll_runtime...' -- it is cleaner and
>> easier to understand that way.
>
> Yes, in fact I have restructured it so that it is only called within
> the decide_unroll and decide_peel
> routines.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c
>> File libgcc/libgcov.c (right):
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode832
>> libgcc/libgcov.c:832: #define CUM_CUTOFF_PERCENT_TIMES_10 999
>> Ok now but it should be controllable at compile time with a --param --
>> recorded in the binary;
>
> ok
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode839
>> libgcc/libgcov.c:839: for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE;
>> t_ix++)
>> There does not seem a need for a loop -- only t_ix == GCOV_COUNTER_ARCS
>> is summable.
>
> i wanted to write the code generically, so it would be forward
> compatible and match the rest of the code.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode848
>> libgcc/libgcov.c:848: cum_cutoff = (cs_ptr->sum_all * cutoff_perc)/1000;
>> Overflow possibility?
>
> Good point. I changed this to do the divide first and then the
> multiply. Won't have any noticeable effect given the large threshold
> used by the consumer of this info.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode854
>> libgcc/libgcov.c:854: value_array = (gcov_type *) malloc
>> (sizeof(gcov_type)*cs_ptr->num);
>> space
>
> done
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode860
>> libgcc/libgcov.c:860: for (i = 0, ctr_info_ix = 0; i < t_ix; i++)
>> No need for this -- the index for ARC counter is always 0 (add assert)
>
> I thought about using that to simplify this code, but was trying to be
> as generic as possible for forward compatibility in case anything
> changed. What do you think?
>
>> -- so either skip (use merge function for 47 and mask for 46) or use 0.
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode864
>> libgcc/libgcov.c:864: }
>> in gcc_46 and before, the counters may not be allocated, and it should
>> not
>> directly accessed using t_ix. It needs to be guarded with
>>
>> if ((1 << t_ix) & gi_ptr->ctr_mask)
>
> The code for 4_6 is simpler and looks different because the counters
> are structured differently.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode875
>> libgcc/libgcov.c:875: gcc_assert (index + ci_ptr->num <= cs_ptr->num);
>> Need to relax this  a little by skipping -- profiling dumping is known
>> to be 'flaky'
>
> fixed, now handle this case gracefully.
>
>>
>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode1103
>> libgcc/libgcov.c:1103: cs_prg->sum_max += cs_tprg->run_max;
>> For multiple runs, how should num_to_cutoff merged? Pick the larger
>> value?
>
> that's a good idea - fixed.
>
> New patch coming shortly.
>
> Thanks,
> Teresa
>
>>
>> http://codereview.appspot.com/6282045/
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Teresa Johnson]

On Wed, Jun 6, 2012 at 3:12 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> The patch looks like 4_7 based which can be different from 4_6 based
>> one. Do you have the latter one for review?
>
> Hi,
> the code determining where program spend most of time in more sane way than
> current hot/cold decision logic is long time on my TODO. My plan was to count
> number of gimple statments executed at link time in ipa-profile pass and then
> scale the logic based on it. But siply sorting gcov counts ignoring the fact
> htat we have no clue how many instruction given counter controls don't seem
> like bad logic. (especially for non-LTO compilations).
> Any plans fo rmainline version?

Yes, I would like to put this in mainline as well. I'll send that
patch out after I have added this to the google branches.

Thanks,
Teresa

> Honza



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

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Teresa Johnson]

On Wed, Jun 6, 2012 at 3:09 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Wed, Jun 6, 2012 at 2:02 PM, Teresa Johnson <tejohnson@google.com> wrote:
>> On Tue, Jun 5, 2012 at 11:46 AM,  <davidxl@google.com> wrote:
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h
>>> File gcc/gcov-io.h (right):
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode544
>>> gcc/gcov-io.h:544: gcov_unsigned_t sum_cutoff_percent;/* sum_all cutoff
>>> percentage computed
>>> Is there a need to record this?
>>
>> It isn't used by by the profile-use compile, I thought it might be
>> useful to have in knowing how to interpret the count. But I can remove
>> it.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/gcov-io.h#newcode546
>>> gcc/gcov-io.h:546: gcov_unsigned_t num_to_cutoff;/* number of counters
>>>
>>> to reach above cutoff.  */
>>> This should have a better name -- e.g., hot_arc_count.
>>
>> Ok. I have changed it to "num_hot_counters" to be less specific to the
>> type of counter the the summary represents.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c
>>> File gcc/loop-unroll.c (right):
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode196
>>> gcc/loop-unroll.c:196: or peeled loop.  */
>>> Add more documentation here: 1) for program size < threshold, do not
>>> limit 2) for    threshold < psize < 2* threshold, tame the max allows
>>> peels/unrolls according to hotness; 3) for huge footprint programs,
>>> disable it (by ...).
>>
>> done.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode197
>>> gcc/loop-unroll.c:197: static limit_type
>>> Blank line.
>>
>> fixed
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode233
>>> gcc/loop-unroll.c:233: gcc_assert(profile_info->sum_all > 0);
>>> Do not assert on profile data -- either bail or emit info.
>>
>> done.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode237
>>> gcc/loop-unroll.c:237: if (profile_info->num_to_cutoff <
>>> size_threshold*2) {
>>> space
>>
>> where? i added space around the "*" and also fixed up the curly brace
>> formatting in this block.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode238
>>> gcc/loop-unroll.c:238: /* For appliations that are less than twice the
>>> codesize limit, allow
>>> applications
>>
>> fixed
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode532
>>> gcc/loop-unroll.c:532: limit = limit_code_size(loop, &codesize_divisor);
>>> space.
>>
>> fixed (at new callsites, see below)
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/gcc/loop-unroll.c#newcode1095
>>> gcc/loop-unroll.c:1095: int codesize_divisor)
>>> This parameter is not documented. The name of the parameter is also not
>>> ideal -. Is it possible to not change the interfaces? -- i.e., split
>>> limit_code_size into two helper functions one to get the the suppress
>>> flag as before, and the other gets the limit_factor which is called
>>> inside each function 'decide_unroll_runtime...' -- it is cleaner and
>>> easier to understand that way.
>>
>> Yes, in fact I have restructured it so that it is only called within
>> the decide_unroll and decide_peel
>> routines.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c
>>> File libgcc/libgcov.c (right):
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode832
>>> libgcc/libgcov.c:832: #define CUM_CUTOFF_PERCENT_TIMES_10 999
>>> Ok now but it should be controllable at compile time with a --param --
>>> recorded in the binary;
>>
>> ok
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode839
>>> libgcc/libgcov.c:839: for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE;
>>> t_ix++)
>>> There does not seem a need for a loop -- only t_ix == GCOV_COUNTER_ARCS
>>> is summable.
>>
>> i wanted to write the code generically, so it would be forward
>> compatible and match the rest of the code.
>
>
> Here it is different -- if there were multiple summable counters,
> adding/sorting them together may not make sense. It is the arc counter
> that needs to be collected.

ok, will change.

>
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode848
>>> libgcc/libgcov.c:848: cum_cutoff = (cs_ptr->sum_all * cutoff_perc)/1000;
>>> Overflow possibility?
>>
>> Good point. I changed this to do the divide first and then the
>> multiply. Won't have any noticeable effect given the large threshold
>> used by the consumer of this info.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode854
>>> libgcc/libgcov.c:854: value_array = (gcov_type *) malloc
>>> (sizeof(gcov_type)*cs_ptr->num);
>>> space
>>
>> done
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode860
>>> libgcc/libgcov.c:860: for (i = 0, ctr_info_ix = 0; i < t_ix; i++)
>>> No need for this -- the index for ARC counter is always 0 (add assert)
>>
>> I thought about using that to simplify this code, but was trying to be
>> as generic as possible for forward compatibility in case anything
>> changed. What do you think?
>
> It is fine to keep this with t_ix == arc_counter --

ok

> there is one minor
> bug -- before the loop, the following should be added to skip all the
> functions:
>
> if (!gi_ptr->merge[t_ix])
>  continue;

I convinced myself that I didn't need that because of the check I am
doing earlier:

      cs_ptr = &(sum->ctrs[t_ix]);
      if (!cs_ptr->num)
        continue;

If gi_ptr->merge[t_ix] is null then cs_ptr->num will still be zero.
Should I add this for good measure?

I am retesting and will send out later tonight, along with the 4_6 patch.

Thanks,
Teresa

>
>
> David
>
>>
>>> -- so either skip (use merge function for 47 and mask for 46) or use 0.
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode864
>>> libgcc/libgcov.c:864: }
>>> in gcc_46 and before, the counters may not be allocated, and it should
>>> not
>>> directly accessed using t_ix. It needs to be guarded with
>>>
>>> if ((1 << t_ix) & gi_ptr->ctr_mask)
>>
>> The code for 4_6 is simpler and looks different because the counters
>> are structured differently.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode875
>>> libgcc/libgcov.c:875: gcc_assert (index + ci_ptr->num <= cs_ptr->num);
>>> Need to relax this  a little by skipping -- profiling dumping is known
>>> to be 'flaky'
>>
>> fixed, now handle this case gracefully.
>>
>>>
>>> http://codereview.appspot.com/6282045/diff/1/libgcc/libgcov.c#newcode1103
>>> libgcc/libgcov.c:1103: cs_prg->sum_max += cs_tprg->run_max;
>>> For multiple runs, how should num_to_cutoff merged? Pick the larger
>>> value?
>>
>> that's a good idea - fixed.
>>
>> New patch coming shortly.
>>
>> Thanks,
>> Teresa
>>
>>>
>>> http://codereview.appspot.com/6282045/
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413



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

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[Teresa Johnson]

On Thu, Jun 7, 2012 at 12:05 AM,  <davidxl@google.com> wrote:
> other than the update issue, good for google branch.
>
> David
>
>
> http://codereview.appspot.com/6282045/diff/4003/libgcc/libgcov.c
> File libgcc/libgcov.c (right):
>
> http://codereview.appspot.com/6282045/diff/4003/libgcc/libgcov.c#newcode1127
> libgcc/libgcov.c:1127: cs_prg->num_hot_counters =
> cs_tprg->num_hot_counters;
> Should the update be outside the if (!cs_prg->runs++)

You are right - with the change I made to take the max it should be
outside the if. Fixed, retested and committed to google/main.

Backport to google 4_7 and 4_6 going in shortly.

Thanks,
Teresa

>
> http://codereview.appspot.com/6282045/



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

Re: [google] New fdo summary-based icache sensitive unrolling (issue 6282045)

[davidxl]

other than the update issue, good for google branch.

David


http://codereview.appspot.com/6282045/diff/4003/libgcc/libgcov.c
File libgcc/libgcov.c (right):

http://codereview.appspot.com/6282045/diff/4003/libgcc/libgcov.c#newcode1127
libgcc/libgcov.c:1127: cs_prg->num_hot_counters =
cs_tprg->num_hot_counters;
Should the update be outside the if (!cs_prg->runs++)

http://codereview.appspot.com/6282045/