[PATCH] D22491: Added ThinLTO inlining statistics

Easwaran Raman via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 19 13:28:40 PDT 2016


eraman added inline comments.

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:1
@@ +1,2 @@
+#ifndef LLVM_TRANSFORMS_IPO_INLINERSTATS_H
+#define LLVM_TRANSFORMS_IPO_INLINERSTATS_H
----------------
Transforms/Utils is a better place to have these two files.  I will also rename it to ThinLTOInliner stats because it is mostly specific to ThinLTO. Actually, you can get rid of the header file and declare getInlinerStatistics in, say, InlinerPass.h and move the class to the .cpp file. 

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:10
@@ +9,3 @@
+/// InlinerStatistics - calculating and dumping statistics on performed inlines.
+/// It calculates statistics summarized stats like:
+/// (1) Number of inlined imported functions,
----------------
"It calculates and dumps the following stats" is more readable

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:16
@@ +15,3 @@
+/// all performed inlines, and second one only the functions that really have
+/// been inlined to some not imported function. Because llvm uses bottom-up
+/// inliner, it is possible to e.g. import function A, B and then inline B to A,
----------------
"inlined to not imported function" -> "eventually inlined to a function in the importing module"

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:19
@@ +18,3 @@
+/// and after this A might be too big to be inlined into some other function
+/// that calls it. It calculates the real values by building graph, where
+/// the nodes are functions, and edges are performed inlines.
----------------
"the real values" -> "this statistic"

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:12
@@ +11,3 @@
+
+void InlinerStatistics::addInlinedFunction(Function *Fun, Function *Inlined) {
+  assert(Fun && Inlined);
----------------
Naming the arguments Caller and Callee makes it consistent with the terminology used in the inliner

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:72
@@ +71,3 @@
+void InlinerStatistics::dfs(InlineGraphNode *const GraphNode) {
+  for (auto *const InlinedFunctionNode : GraphNode->InlinedFunctions) {
+    InlinedFunctionNode->NumberOfRealInlines++;
----------------
I think the graph can have cycle. Consider a mutual recursion A->B->A. Let's say B gets inlined into A. This would normally result in a self-recursion (A->A). This would prevent A from being inlined into B. But, if we are in a situation where B first gets inlined into A and the A->A call is DCEd. Then A could be inlined into B causing a cycle in your InlineGraph.



================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:73
@@ +72,3 @@
+  for (auto *const InlinedFunctionNode : GraphNode->InlinedFunctions) {
+    InlinedFunctionNode->NumberOfRealInlines++;
+    dfs(InlinedFunctionNode);
----------------
Let's say A and B are local functions (functions in the importing module) and C is an imported function and there is a call chain A->B->C. Let's say C first gets inlined into B and then B gets inlined into A. You'll have nodes for A, B and C and A and B are in the NonExternalFunctions. You'll dfs from both A and B. NumberofRealInlines of B will be 2 which not what you want. 


https://reviews.llvm.org/D22491





More information about the llvm-commits mailing list