On Wed, Jun 6, 2012 at 3:09 PM, Xinliang David Li <davi...@google.com> wrote: > 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. >> >>> >>> 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 | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
