[PATCH] D22491: Added ThinLTO inlining statistics

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Haven't gone through the details of the dumping routine yet, but here are a few initial high comments.


================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:12
@@ +11,3 @@
+/// (1) Number of inlined imported functions,
+/// (2) Number of real inlined imported functions
+/// (3) Number of real not external inlined functions
----------------
Perhaps instead of referring to these as a "real inline" it would be clearer to say "inline to importing module" or something more descriptive like that.

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:22
@@ +21,3 @@
+/// Then starting from non external functions that have some inlined calls
+/// inside, it walks to every inlined function and increment counter.
+///
----------------
mehdi_amini wrote:
> It is not clear to me why you need a graph and can't just increment counters as the inlined moves on?
This is to distinguish between inlines that eventually make it into a non-imported function (i.e. what Piotr refers to as a "real inline" and I am suggesting something like "inline to importing module"). I.e. if we have call chain A->B->C where A was originally in the module (non-imported) and B/C were imported, when we inline C into B we don't know if they will ultimately be inlined into A. Presumably there could be another SCC that we perform inlines on in between the B->C and A->BC inlines so we can't easily track this without some side structure.

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:30
@@ +29,3 @@
+private:
+  struct InlineGraphNode {
+    InlineGraphNode() = default;
----------------
structure needs description.

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:55
@@ +54,3 @@
+  using SortedNodesTy = std::vector<NodesMapTy::value_type>;
+  // Clears NodesMap and returns vector of elements sorted by
+  // (-NumberOfInlines, -NumberOfRealInlines, FunctioName).
----------------
Why also clear map?

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:56
@@ +55,3 @@
+  // Clears NodesMap and returns vector of elements sorted by
+  // (-NumberOfInlines, -NumberOfRealInlines, FunctioName).
+  SortedNodesTy getSortedNodes();
----------------
s/FunctioName/FunctionName/

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:59
@@ +58,3 @@
+
+private:
+  NodesMapTy NodesMap;
----------------
Unnecessary private here (already in private section)

================
Comment at: lib/Transforms/IPO/Inliner.cpp:52
@@ +51,3 @@
+static cl::opt<bool>
+    EnableInlineGraphStats("enable-import-graph-stats", cl::init(false),
+                           cl::Hidden, cl::desc("Enable inline graph stats"));
----------------
How about drop "Enable"/"enable" from the stats for brevity, but add "inliner" for clarity. I.e. something like -inliner-function-import-stats (not sure the "graph" adds much here, so remove that?).

================
Comment at: lib/Transforms/IPO/Inliner.cpp:56
@@ +55,3 @@
+static cl::opt<bool> EnableListStats(
+    "enable-list-stats", cl::init(false), cl::Hidden,
+    cl::desc("Enable printing of statistics for each inlined function"));
----------------
Ditto here. How about -inliner-function-import-verbose-stats or maybe make the earlier option take a "=verbose" sub option? I.e. it would be cl::opt<std::string> and default to "", and support "verbose" as a value.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:89
@@ -80,2 +88,3 @@
                                  int InlineHistory, bool InsertLifetime) {
+  // Calle and Caller information will be gone in CS after inlining.
   Function *Callee = CS.getCalledFunction();
----------------
s/Calle/Callee/. When you say "gone" do you mean that the fields are cleared and become null?

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:1
@@ +1,2 @@
+#include "llvm/Transforms/IPO/InlinerStats.h"
+#include "llvm/Support/Debug.h"
----------------
File needs block comment at top.

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:17
@@ +16,3 @@
+  if (!FunNode.Imported)
+    NonExternalFunctions.push_back(Fun);
+
----------------
s/NonExternal/NonImported/?

Also, isn't it possible that we would have seen Fun before, in which case it might get pushed here multiple times? Can the NonExternalFunctions list be built by a single walk over the module rather than checking for every single inline? Looking at its use, it appears to only be used once during dumping in calculateRealInlines, maybe we can just walk the functions in the module there instead of keeping a list.


https://reviews.llvm.org/D22491