[PATCH] D22491: Added ThinLTO inlining statistics

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Like the new handling for inserting the Nodes, looks much cleaner.


================
Comment at: include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h:63
@@ +62,3 @@
+    bool Visited = false;
+    /// Store name because the function pointer might be dead.
+    std::string FunctionName;
----------------
Instead, I like Easwaran's suggestion to make NodesMap a StringMap. 

Then NonImportedCallers should be a vector of strings. Note that StringMap stores a copy of the key when you insert, so you could store the resulting StringRef (returned when the StringMap entry is inserted) in the NonImportedCallers vector instead of a std::string copy.

It is cleaner than using keys that are pointers to possibly deleted memory.

================
Comment at: include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h:95
@@ +94,3 @@
+  /// InlineGraphNode used since the node pointers are also saved in the
+  /// InlinedCallees vector. Ff it would store InlineGraphNode instead then the
+  /// address of the node would not be invariant.
----------------
s/Ff/If/

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:61
@@ +60,3 @@
+  int32_t ImportedFunctions = 0;
+  // FIXME: We are not counting all the functions that were inlined because
+  // some of them could be removed after inlining.
----------------
Good catch. Call this from the ImportedFunctionsInliningStatistics constructor and save the results there instead.


https://reviews.llvm.org/D22491