[PATCH] D41297: [ThinLTO] Implement summary visualizer

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:77
+  /// Summary string representation. Valid for combined summaries
+  StringRef Name;
+
----------------
tejohnson wrote:
> 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). 
>  Who owns the string?

My understanding is that linker owns this string because it loads bitcode file to memory and keeps it there during entire link phase. This StringRef points to strtab of the module, which is, to my understanding, is a region within MemoryBuffer owned by lld. Not so sure about gold.

I've found a problem though with VST_CODE_ENTRY/VST_CODE_FINENTRY. When they're being parsed their names are created on stack. This should be fixed.

> I wonder if we can keep these names around somewhere under a debugging option instead (and emit the GUID by default).

Sounds reasonable, if there is no other way. I'll take a look on code reading/writing VST entries 



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:112
+  StringRef name() const {
+    return Ref->second.GV ? Ref->second.GV->getName() : Ref->second.Name;
+  }
----------------
tejohnson wrote:
> 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.
>  Although it may be good to support emitting this for the per-module summary as well.

Yep, that's what I was thinking about.


https://reviews.llvm.org/D41297