[PATCH] D22491: Added ThinLTO inlining statistics

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:43
@@ +42,3 @@
+    int16_t NumberOfInlines = 0;
+    /// Number of inlines that leads to not imported function.
+    /// Computed based on graph search.
----------------
Maybe "Number of inlines into non imported function (possibly indirect via intermediate inlines)"

================
Comment at: lib/Transforms/IPO/Inliner.cpp:89
@@ -80,2 +88,3 @@
                                  int InlineHistory, bool InsertLifetime) {
+  // Callee and Caller information will be gone in CS after inlining.
   Function *Callee = CS.getCalledFunction();
----------------
Maybe then change comment to say "Callee and Caller will be set to null in CS after inlining"

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:22
@@ +21,3 @@
+  if (!CallerNode.Imported && !CalleeNode.Imported) {
+    // Direct inline form not imported callee to not imported caller. It is
+    // basically small optimization to not put this into graph, so for modules
----------------
s/form/from/

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:24
@@ +23,3 @@
+    // basically small optimization to not put this into graph, so for modules
+    // without imported functions there won't be any graph traversal.
+    CalleeNode.NumberOfRealInlines++;
----------------
Even with imported functions, by not putting intra-module inlines in the graph it should reduce the size and the traversal overhead. Although I would expect that most intra module inlines would have been done already in the compile step. Maybe just note that in your comment.

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:52
@@ +51,3 @@
+      InlinedNotImportedFunctionsToImportingModuleCount =
+          (Node.second.NumberOfRealInlines > 0) * 1;
+    }
----------------
Oh got it, I missed the fact that we only have the callee info here and aren't walking from the caller.

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:57
@@ +56,3 @@
+    // No more inlined functions.
+    if (Node.second.NumberOfInlines == 0)
+      break;
----------------
I would remove this early exit as it makes assumptions about the sorting done, and if the sorting algorithm changes this would need to change too. Maybe just continue so we don't get a message under EnableListStats below when there are no inlines.

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:70
@@ +69,3 @@
+         << InlinedImportedFunctionsCount
+         << "\nNumber of inlined imported functions to importing module: "
+         << InlinedImportedFunctionsToImportingModuleCount
----------------
s/inlined imported functions to/imported functions inlined into/

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:72
@@ +71,3 @@
+         << InlinedImportedFunctionsToImportingModuleCount
+         << "\nNumber of inlined not imported functions to importing module: "
+         << InlinedNotImportedFunctionsToImportingModuleCount << "\n";
----------------
s/inlined not imported functions to/non-imported functions inlined into/


https://reviews.llvm.org/D22491