On Wed, Jun 6, 2012 at 2:02 PM, Teresa Johnson <tejohn...@google.com> wrote: > On Tue, Jun 5, 2012 at 11:46 AM, <davi...@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 | tejohn...@google.com | 408-460-2413
