[PATCH] D22491: Added ThinLTO inlining statistics

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Looks pretty close. Just some minor comments below.


================
Comment at: include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h:9
@@ +8,3 @@
+//===----------------------------------------------------------------------===//
+// Generating inliner statistics for imported functions, mostly usefull for
+// ThinLTO.
----------------
s/usefull/useful/

================
Comment at: lib/Transforms/IPO/Inliner.cpp:597
@@ +596,3 @@
+    ImportedFunctionsStats.dump(InlinerFunctionImportStats ==
+                                    InlinerFunctionImportStatsOpts::Verbose,
+                                CG.getModule());
----------------
This indentation doesn't look right to me - make sure you clang format.

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:9
@@ +8,3 @@
+//===----------------------------------------------------------------------===//
+// Generating inliner statistics for imported functions, mostly usefull for
+// ThinLTO.
----------------
s/usefull/useful/

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:50
@@ +49,3 @@
+}
+static std::pair<int32_t, int32_t> getFunctionsCount(const Module &M) {
+  int32_t AllFunctions = 0;
----------------
Add empty line above.

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:131
@@ +130,3 @@
+      << getStatString("inlined functions", InlinedFunctionsCount, AllFunctions)
+      << getStatString("imported functions inlined",
+                       InlinedImportedFunctionsCount, ImportedFunctions)
----------------
suggest adding "anywhere" at the end to distinguish from below stat.

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:136
@@ +135,3 @@
+                       InlinedImportedFunctionsCount)
+      << getStatString("Inlined not imported functions",
+                       InlinedNotImportedFunctionsCount, NotImportedFuncCount)
----------------
Make this consistent with other lines, e.g. maybe "non-imported functions inlined anywhere".

================
Comment at: test/Transforms/Inline/inline_stats.ll:17
@@ +16,3 @@
+; CHECK: Inlined not imported functions: 1 [33.33%]
+; CHECK: non-imported functions inlined into importing module: 1 [100%]
+
----------------
It isn't clear what the percentages represent (e.g. some are percent of total # functions and some are percent of inlined functions). Probably need to add a tag - e.g.:

; CHECK: inlined functions: 5 [50% of all functions]
; CHECK: imported functions inlined: 4 [57.14% of imported functions]
; CHECK: imported functions inlined into importing module: 3 [75% of inlined imported functions]
; CHECK: Inlined not imported functions: 1 [33.33% of non-imported functions]
; CHECK: non-imported functions inlined into importing module: 1 [100% of non-imported inlined functions]

However, after typing that out, I think for the stat relating to how many are inlined into the importing module, it would be much more useful to have the percentage in terms of the total number of that type of function. E.g. instead of expressing the "imported functions inlined into importing module" as a percentage of "imported functions inlined", it would be more useful to have it be a percentage of "imported functions". Ditto for non-imported functions inlined into importing module.


https://reviews.llvm.org/D22491