[PATCH] D22491: Added ThinLTO inlining statistics

[Easwaran Raman via llvm-commits]

eraman added a comment.

Generally looks good. Most of the comments are minor. Please add a test case for the non-verbose case.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:65
@@ +64,3 @@
+               clEnumValEnd),
+    cl::Hidden, cl::desc("Enable ThinLTO specific inliner stats"));
+} // namespace
----------------
"Inliner stats for imported functions" or something like that. Since the option name does not have ThinLTO, remove it from the description.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:96
@@ +95,3 @@
+static bool InlineCallIfPossible(
+    Pass &P, CallSite CS, InlineFunctionInfo &IFI,
+    InlinedArrayAllocasTy &InlinedArrayAllocas, int InlineHistory,
----------------
To reiterate, the problem is that the call instruction does not exist and calling any methods on that CallInst accesses freed memory. CallSite does not explicitly store Caller and Callee and so there is no question of "Callee and Caller will be set to null". 

================
Comment at: lib/Transforms/Utils/CMakeLists.txt:19
@@ -18,2 +18,3 @@
   InlineFunction.cpp
+        ImportedFunctionsInliningStatistics.cpp
   InstructionNamer.cpp
----------------
Indentation.

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:24
@@ +23,3 @@
+ImportedFunctionsInliningStatistics::ImportedFunctionsInliningStatistics() {
+  NonImportedCallers.reserve(200);
+}
----------------
Inliner contains an object of ImportedFunctionsInliningStatistics. By default you are not generating statistics and yet this will call reserve unnecessarily.  Not a big deal - we are talking about <2KB per module compilation here, but still unnecessary. 

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:106
@@ +105,3 @@
+    if (Node.second.Imported) {
+      InlinedImportedFunctionsCount += (Node.second.NumberOfInlines > 0) * 1;
+      InlinedImportedFunctionsToImportingModuleCount +=
----------------
Node.second.NumberOfInlines is > 0 here, so you don't need the (Node.second.NumberOfInlines > 0) * 1. You could simply increment the count by 1.

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:108
@@ +107,3 @@
+      InlinedImportedFunctionsToImportingModuleCount +=
+          (Node.second.NumberOfRealInlines > 0) * 1;
+    } else {
----------------
No need to do * 1

================
Comment at: test/Transforms/Inline/inline_stats.ll:1
@@ +1,2 @@
+; RUN: opt -S -inline -inliner-function-import-stats=verbose < %s 2>&1 | FileCheck %s
+
----------------
This doesn't test the non-verbose case. Just use this test case and use a different filecheck prefix. 


https://reviews.llvm.org/D22491