[PATCH] D22491: Added ThinLTO inlining statistics

[Piotr Padlewski via llvm-commits]

Prazek added inline comments.

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:22
@@ +21,3 @@
+/// Then starting from non external functions that have some inlined calls
+/// inside, it walks to every inlined function and increment counter.
+///
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > It is not clear to me why you need a graph and can't just increment counters as the inlined moves on?
> This is to distinguish between inlines that eventually make it into a non-imported function (i.e. what Piotr refers to as a "real inline" and I am suggesting something like "inline to importing module"). I.e. if we have call chain A->B->C where A was originally in the module (non-imported) and B/C were imported, when we inline C into B we don't know if they will ultimately be inlined into A. Presumably there could be another SCC that we perform inlines on in between the B->C and A->BC inlines so we can't easily track this without some side structure.
indeed. I am gonna change that comment to be more clear

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:55
@@ +54,3 @@
+  using SortedNodesTy = std::vector<NodesMapTy::value_type>;
+  // Clears NodesMap and returns vector of elements sorted by
+  // (-NumberOfInlines, -NumberOfRealInlines, FunctioName).
----------------
tejohnson wrote:
> Why also clear map?
because I am moving all the pairs from map to vector, so either I will do a copy of everything or I will move all the stuff

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:59
@@ +58,3 @@
+
+private:
+  NodesMapTy NodesMap;
----------------
tejohnson wrote:
> Unnecessary private here (already in private section)
Yes, but this label splits private functions from private members. It is not necessary, but the code is more readable this way.
I also think I saw usages like this in clang.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:89
@@ -80,2 +88,3 @@
                                  int InlineHistory, bool InsertLifetime) {
+  // Calle and Caller information will be gone in CS after inlining.
   Function *Callee = CS.getCalledFunction();
----------------
tejohnson wrote:
> s/Calle/Callee/. When you say "gone" do you mean that the fields are cleared and become null?
exactly, the fields are set to null after inlining

================
Comment at: lib/Transforms/IPO/InlinerStats.cpp:17
@@ +16,3 @@
+  if (!FunNode.Imported)
+    NonExternalFunctions.push_back(Fun);
+
----------------
tejohnson wrote:
> s/NonExternal/NonImported/?
> 
> Also, isn't it possible that we would have seen Fun before, in which case it might get pushed here multiple times? Can the NonExternalFunctions list be built by a single walk over the module rather than checking for every single inline? Looking at its use, it appears to only be used once during dumping in calculateRealInlines, maybe we can just walk the functions in the module there instead of keeping a list.
Ok so there are two things
1. It is true that the function might be added here multiple times. I can solve it easily by sorting the vector and then by adding +n to the counter in DFS, where n is how many times Fun have been pushed.

2. I think walking all the functions would be much slower. I don't expect this list to be as big as all the functions. probably 5-25% in thinlto. So rather than iterating all the functions, and having some boolean to distinguish if I should start DFS from this or not, I can do the option (1) which I think is better.


https://reviews.llvm.org/D22491