[PATCH] D22491: Added ThinLTO inlining statistics

[Easwaran Raman via llvm-commits]

eraman added inline comments.

================
Comment at: include/llvm/Transforms/IPO/InlinerPass.h:73
@@ -71,4 +72,3 @@
   /// call this function unconditionally.
   bool inlineCalls(CallGraphSCC &SCC);
 private:
----------------
Formatting - the empty line has been removed after this.

================
Comment at: include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h:89
@@ +88,3 @@
+  /// This map manage life of all InlineGraphNodes. Pointer to InlineGraphNode
+  /// as value is required because DenseMap invalidates the iterators.
+  /// The Function under *value may be invalid due to removal of Functions
----------------
The comment is not very clear to me. What does the value type got to with invalidation?

================
Comment at: include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h:97
@@ +96,3 @@
+  /// before dumping stats.
+  llvm::DenseMap<const Function *, std::string> NameMap;
+};
----------------
I'm missing something here. Why not make the function name the key to the other map and remove this NameMap? This will change NonImportedCallers as well...

================
Comment at: lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp:30
@@ +29,3 @@
+  NodesMapTy::mapped_type *ValueLookup; // To avoid multiple map lookups.
+  InlineGraphNode &CallerNode =
+      (ValueLookup = &NodesMap[&Caller])
----------------
I find this quite difficult to read. I prefer using if(...) here instead of ternary operator. Also, you don't need to make ValueLookup a pointer to mapped_type. Keeping it a reference will remove one  * operator's use. 

Also add a comment saying that the second lookup could result in the resizing map and that's why you can't just keep a reference to the uniquq_ptr returned by the lookup.




  


https://reviews.llvm.org/D22491