[PATCH] D22491: Added ThinLTO inlining statistics

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Couple minor comments on the comments, but otherwise looks ok. I'm not convinced the complexity of the new handling in recordInlineto avoid the extra map lookup is necessary since this is just stat counting code not on by default though, but I don't have a strong objection to it either.


================
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
----------------
Not so much because iterator is invalidated (although an iterator being invalidated would be a side effect of the same issue)  - it is because the DenseMap value may be moved, and you need to store pointers to the InlineGraphNodes elsewhere. IIUC that is. You could just change this to "Unique pointer to InlineGraphNode used since the node pointers are also saved in the InlinedCallees vector."

================
Comment at: include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h:91
@@ +90,3 @@
+  /// The Function under *value may be invalid due to removal of Functions
+  /// during inlining.
+  NodesMapTy NodesMap;
----------------
Might want to note that this is ok because you don't dereference the key (using the NameMap to avoid needing to).


https://reviews.llvm.org/D22491