[PATCH] D60977: [llvm-profdata] Add overlap command to compute similarity b/w two profile files

[Rong Xu via Phabricator via llvm-commits]

xur marked 7 inline comments as done.
xur added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-profdata.rst:286
+
+.. option:: -output=output, -o=output
+
----------------
davidxl wrote:
> -o=output or -o output
it does take -output.
may be "-output=output, -output output, -o=output, or -o output"? (it's a little verbose). I actually copied this line from merge and show subcommand


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:1210
+void OverlapStats::dump(raw_fd_ostream &OS) const {
+  OS << "EdgeCount: (# of functions)\n";
+  OS << "  Overlap:\t" << format("%.3f%%", Overlap.EdgeCount * 100) << " ("
----------------
davidxl wrote:
> For function level dump, the # of function is redundant -- perhaps change it to # of counters
> 
> The overall dump format looks better with this:
> 1) program level:
> 
>       Edge Profile:   (overlap, # of functions )
>                                 80%,      10
>        Incall Profile:  (overlap, # of functions)
>                                 70%,       10
>        ...
> 2) function level
>        Edge Profile: (overlap, # of counters)
>                                75%,     10
>        ...
> 
I will change to this format. 

add number of counts to function level is a good idea.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:1211
+  OS << "EdgeCount: (# of functions)\n";
+  OS << "  Overlap:\t" << format("%.3f%%", Overlap.EdgeCount * 100) << " ("
+     << Overlap.NumFuncs << ")\n";
----------------
davidxl wrote:
> The header and content does not match in fields.
why this does not match?
the first column is the percent score and the second is the number if functions in this category. I don't see the problem.


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:190
 
+void InstrProfWriter::overlapRecord(NamedInstrProfRecord &&Other,
+                                    OverlapStats &Overlap,
----------------
davidxl wrote:
> Why is this a writer method? 
because this reuses the merge framework (load the base profile to write context) and load the second one to do the overlap calculation. 


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:223
+    if (FuncOverlap.Valid) {
+      OS << "Function level overlap info of " << I.Name << " (hash=" << I.Hash
+         << ")\n";
----------------
davidxl wrote:
> The title print including the summary can be pushed into FuncOverlap::dump() method
This would require to put a flag in the structure to distinguish whether this is function level or program level.  It can be done.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:228
+  }
+  OS << "== Program level overlap info ==\n"
+     << "  BaseFilename: " << BaseFilename
----------------
davidxl wrote:
> The title can be pushed into FunctionOverlap::dump() method.
This would require add both profile file name to the structure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60977/new/

https://reviews.llvm.org/D60977