On Wed, Sep 19, 2012 at 3:03 PM, <de...@google.com> wrote: > FYI. > > Dehao
Thanks. Responses below. New patch being uploaded shortly. Teresa > > > https://codereview.appspot.com/6489092/diff/4001/gcc/common.opt > File gcc/common.opt (right): > > https://codereview.appspot.com/6489092/diff/4001/gcc/common.opt#newcode1688 > gcc/common.opt:1688: -fpmu-profile-use=[pmuprofile.gcda] The pmu > > profile data file to use for pmu feedback. > Looks like the default value is not implemented. Fixed. > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c > File gcc/coverage.c (right): > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c#newcode777 > gcc/coverage.c:777: +static void read_pmu_file (const char* > da_file_name) > This function is very large. How about splitting it into read_pmu_file > and process_pmu_file (the second one builds the hash tables, etc.) Done. Moved the hash table code into process_pmu_data, called by read_pmu_file. > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c#newcode781 > gcc/coverage.c:781: + brm_infos_t* brm_infos = > &pmu_global_summary.brm_infos; > For consistency, please move the "*" after the space. (many places in > this file) Fixed. > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c#newcode846 > gcc/coverage.c:846: + unsigned length = gcov_read_unsigned (); > Please use gcov_unsigned_t Fixed. > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c#newcode847 > gcc/coverage.c:847: + unsigned long base = gcov_position (); > Please use gcov_position_t Fixed. > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c#newcode940 > gcc/coverage.c:940: + there should only be one entry per > > filename and line number */ > add a gcc_assert in the else path? Yes, added an assert, also added it to the branch mispredict path as well, to verify that the entry (now returned by a helper func) does not already have that type of pmu data. Actually, after testing with an example pmuprofile.gcda file I generated, I removed these asserts. Looks like the tool is generating pmu entries for addresses it can't attribute to a known source file to the same source position (file:line of null:0). This should probably be fixed in the tool that generates the gcda file, but for now rather than assert or try to filter these out in the compiler, I am just ignoring the fact that there may be duplicates. > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c#newcode966 > gcc/coverage.c:966: + } > The above two loops looks similar, can they be abstracted out? I pulled out the hash table entry lookup/setup code into a new helper, get_pmu_hash_entry. > > https://codereview.appspot.com/6489092/diff/4001/gcc/coverage.c#newcode2573 > gcc/coverage.c:2573: + if (pmu_profile_data != 0 && TDF_PMU) > if (pmu_profile_data != NULL && ...) > > or > > if (pmu_profile_data && ...) This code has changed due to the new option handling I added, to deal with the default filename. The check essentially looks like your second option though. > > https://codereview.appspot.com/6489092/diff/4001/gcc/gcov-io.h > File gcc/gcov-io.h (right): > > https://codereview.appspot.com/6489092/diff/4001/gcc/gcov-io.h#newcode705 > gcc/gcov-io.h:705: /* Cumulative pmu data */ > Seems this data structure should be moved into coverage.c. Yes, done. > > https://codereview.appspot.com/6489092/diff/4001/gcc/gcov.c > File gcc/gcov.c (right): > > https://codereview.appspot.com/6489092/diff/4001/gcc/gcov.c#newcode2353 > gcc/gcov.c:2353: + > remove blank line Fixed. > > https://codereview.appspot.com/6489092/diff/4001/gcc/gimple-pretty-print.c > File gcc/gimple-pretty-print.c (right): > > https://codereview.appspot.com/6489092/diff/4001/gcc/gimple-pretty-print.c#newcode1585 > gcc/gimple-pretty-print.c:1585: static void > This function is very much like dump_pmu. Can we jus call dump_pmu here? Agreed. Done. > > https://codereview.appspot.com/6489092/diff/4001/gcc/tree-pretty-print.c > File gcc/tree-pretty-print.c (right): > > https://codereview.appspot.com/6489092/diff/4001/gcc/tree-pretty-print.c#newcode28 > gcc/tree-pretty-print.c:28: #include "basic-block.h" > why need to include his header? It defines gcov_type for the compiler. > > https://codereview.appspot.com/6489092/diff/4001/gcc/tree-pretty-print.c#newcode519 > gcc/tree-pretty-print.c:519: static void > Looks to me that this function should be exported, while > dump_load_latency_details should stay static. Yep, fixed. > > https://codereview.appspot.com/6489092/ -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
