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. > > 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
