[PATCH] D22491: Added ThinLTO inlining statistics

[Easwaran Raman via llvm-commits]

eraman added inline comments.

================
Comment at: include/llvm/Transforms/Utils/InlinerStats.h:1
@@ +1,2 @@
+//===-- InlinerStats.h - Generating inliner statistics ----------*- C++ -*-===//
+//
----------------
As discussed offline, incorporate ThinLTO to the name or at the minimum add comments to indicate that the primary intent is to use with ThinLTO. Perhaps rename it to InlinerFunctionImportStats.h  - as used in the option name.

================
Comment at: include/llvm/Transforms/Utils/InlinerStats.h:16
@@ +15,3 @@
+
+void addInlinedFunctionToInlinerStats(const Function *Caller,
+                                      const Function *Callee);
----------------
Nit: I prefer recordInlining or similar small name, but will leave it to you

================
Comment at: lib/Transforms/IPO/Inliner.cpp:61
@@ +60,3 @@
+    cl::init(InlinerFunctionImportStatsOpts::No),
+    cl::values(clEnumValN(InlinerFunctionImportStatsOpts::Basic, "",
+                          "basic statistics"),
----------------
use the string "Basic" instead of an empty string

================
Comment at: lib/Transforms/IPO/Inliner.cpp:99
@@ -80,2 +98,3 @@
                                  int InlineHistory, bool InsertLifetime) {
+  // Callee and Caller will be set to null in CS after inlining.
   Function *Callee = CS.getCalledFunction();
----------------
You can not call getCallee() or getCalledFunction()  because the call instruction does not exist after inlining and those methods access methods of the call instruction.

As an aside, this comment is not specific to this patch and typically could just add the comment separately (which doesn't need any review). I don't mind it being part of this patch though.

================
Comment at: lib/Transforms/Utils/InlinerStats.cpp:25
@@ +24,3 @@
+/// (1) Number of inlined imported functions,
+/// (2) Number of inlined imporrted functions to importing module (indirect)
+/// (3) Number of inlined non imported functions to importing module (indirect)
----------------
typo: imporrted -> imported

================
Comment at: lib/Transforms/Utils/InlinerStats.cpp:31
@@ +30,3 @@
+/// module (by a chain of inlines). Because llvm uses bottom-up inliner, it is
+/// possible
+/// to e.g. import function A, B and then inline B to A,
----------------
merge the next line here

================
Comment at: lib/Transforms/Utils/InlinerStats.cpp:38
@@ +37,3 @@
+///
+/// If `EnableListStats` is set to true, then it also dumps statistics
+/// per each inlined function, sorted by the greatest inlines count like
----------------
Comment out of sync with the code

================
Comment at: lib/Transforms/Utils/InlinerStats.cpp:55
@@ +54,3 @@
+    /// Incremented every direct inline.
+    int16_t NumberOfInlines = 0;
+    /// Number of inlines into non imported function (possibly indirect via
----------------
I don't expect the number to exceed max value of int16_t in real programs, but perhaps possible in generated test cases. Why not make it int32_t ?

================
Comment at: lib/Transforms/Utils/InlinerStats.cpp:193
@@ +192,3 @@
+InlinerStatistics &getInlinerStatistics() {
+  static InlinerStatistics Stats;
+  return Stats;
----------------
As discussed offline, this is not threadsafe


https://reviews.llvm.org/D22491