[PATCH] D22491: Added ThinLTO inlining statistics

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm/Transforms/Utils/ThinLTOInlinerStats.h:1
@@ +1,2 @@
+//===-- ThinLTOInlinerStats.h - Generating inliner statistics ---*- C++ -*-===//
+//
----------------
"Generating inliner statistics for imported functions"

================
Comment at: include/llvm/Transforms/Utils/ThinLTOInlinerStats.h:16
@@ +15,3 @@
+
+void recordInline(const Function &Caller, const Function &Callee);
+/// Dump stats computed with InlinerStatistics class.
----------------
doxygen comment

================
Comment at: include/llvm/Transforms/Utils/ThinLTOInlinerStats.h:17
@@ +16,3 @@
+void recordInline(const Function &Caller, const Function &Callee);
+/// Dump stats computed with InlinerStatistics class.
+void dumpInlinerStats(bool Verbose);
----------------
s/InlinerStatistics/ThinLTOInlinerStatistics/ (suggested below)

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:61
@@ +60,3 @@
+         << NumberOfRealUniqueInlinedImportedFunctions
+         << "\nNumber of real not external inlined functions: "
+         << NumberOFRealUniqueInlinedNotExternalFunctions << "\n";
----------------
Prazek wrote:
> tejohnson wrote:
> > Should be trivial I think to compute the reverse as well: how many imported functions were not inlined into a non-imported function (possibly via other inlines).
> I am not sure if I should compute it here. The number of all imported functions is counted in importer.
Yes but then the client has to find both stats and subtract rather than get the stat immediately. I think it would be useful to walk over the functions here, and simply count how many have the imported metadata tag, then emit that info here, along with the percentage of imported funcs that were inlined into the importing module and that weren't.

================
Comment at: lib/Transforms/Utils/ThinLTOInlinerStats.cpp:1
@@ +1,2 @@
+//===-- ThinLTOInlinerStats.cpp - Generating inliner statistics -*- C++ -*-===//
+//
----------------
"Generating inliner statistics for imported functions" is clearer. If too long, probably clarify further down in the file head comment block.

================
Comment at: lib/Transforms/Utils/ThinLTOInlinerStats.cpp:26
@@ +25,3 @@
+/// (1) Number of inlined imported functions,
+/// (2) Number of inlined imported functions to importing module (indirect),
+/// (3) Number of inlined non imported functions to importing module (indirect).
----------------
Here and in below line, I think it is clearer to restructure like "Number of imported functions inlined into...".

================
Comment at: lib/Transforms/Utils/ThinLTOInlinerStats.cpp:42
@@ +41,3 @@
+/// - number of performed inlines to importing module
+class InlinerStatistics {
+private:
----------------
s/InlinerStatistics/ThinLTOInlinerStatistics/

================
Comment at: lib/Transforms/Utils/ThinLTOInlinerStats.cpp:91
@@ +90,3 @@
+                                           const Function &Callee) {
+  std::lock_guard<std::mutex> Guard(Mutex);
+  auto &CallerNode = NodesMap[&Caller];
----------------
What is the multithreaded case we are concerned about? Is it when the ThinLTO backend threads are launched in parallel via a ThreadPool? In that case we want each thread to have a completely independent set of stats, which I don't think this mutex provides.


https://reviews.llvm.org/D22491