[PATCH] D22491: Added ThinLTO inlining statistics

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
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;
----------------
Prazek wrote:
> tejohnson wrote:
> > 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.
> I talked with Easwaran and I don't think there is a point changit the map. It will be much slower and won't get any new information, and it will take a time to change everything again.
It's an issue of robustness rather than getting more information. I mentioned earlier too that it seems brittle to use pointers to deleted functions as keys - the code may not try to dereference them now, but someone else may come along and try to do something with the key, dereferencing the pointer to get some additional info, not realizing that it is possibly deallocated. 

If Easwaran is ok with the code as is, then I'm ok with this for now, but something to clean up later. Please add a FIXME?


https://reviews.llvm.org/D22491