Re: [google] Added new dump flag -pmu to display pmu data in pass summaries (issue 6489092)

[Teresa Johnson <tejohnson at google dot com>]

On Wed, Sep 19, 2012 at 3:03 PM,  <dehao@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 | tejohnson@google.com | 408-460-2413