[PATCH] D41297: [ThinLTO] Implement summary visualizer

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the delay! I have one big concern below. Will look at the code that does the graph emission next, I've only skimmed that part.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:77
+  /// Summary string representation. Valid for combined summaries
+  StringRef Name;
+
----------------
I have a concern about keeping this string here. Who owns the string? I guess it is the string table created during bitcode reading. Do those even stay live through reading all input bitcode files and building the combined index? If not, saving a reference here is a problem. If so, then we currently have overhead that isn't typically necessary (we don't operate on IR or the names during the thin link).

I guess you added this to avoid needing the script to annotate these names instead of using the guids. Assuming we need to do something to keep these names live, I wonder if we can keep these names around somewhere under a debugging option instead (and emit the GUID by default). 


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:112
+  StringRef name() const {
+    return Ref->second.GV ? Ref->second.GV->getName() : Ref->second.Name;
+  }
----------------
Note that we'll never have a GV in the combined index case. Although it may be good to support emitting this for the per-module summary as well.


https://reviews.llvm.org/D41297