[PATCH] D41297: [ThinLTO] Implement summary visualizer

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks. Do you have an example of what this looks like now with a couple of modules and some cross-module edges?



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:71
 
 struct GlobalValueSummaryInfo {
   /// The GlobalValue corresponding to this summary. This is only used in
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > Are we size-sensitive here @tejohnson ?
> > Since the GV field is only used in per-module and and name only in combined summary, we could have some sort of union (variant if available).
> Yeah that is a concern. I like the idea of unioning with the GV.
Please union the GV and Name fields as suggested, so that we don't bloat the size. You will likely need to know whether this is a combined index or not when accessing these. Currently we don't have a flag on the index, but one could be added. You can probably get by with checking whether there are any entries in the index's ModulePathStringTable though, since I believe that is empty for the per-module summary index.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:76
 
+  /// Summary string representation. Valid for combined summaries
+  StringRef Name;
----------------
Need to clearly document ownership of this string


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:174
+
+// Get node label in form MXXX_<GUID>. The MXXX prefix is required,
+// because we may have multiple linkonce functions summaries.
----------------
This comment now belongs elsewhere.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:190
+
+// Write definition of node without ValueInfo. This is typically
+// an external function or variable
----------------
Not sure what is meant by "without ValueInfo" - this function takes a ValueInfo and uses it.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:213
+                      int DstMod, GlobalValue::GUID DstId, int Hotness) {
+    ++Hotness; // 0 corresponds to ref edge, 1 to call with unknown hotness, ...
+    static const char *EdgeAttrs[] = {
----------------
Rename to something like TypeOrHotness instead?


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:281
+
+        Draw(SummaryIt.first, AliaseeId ? AliaseeId : AliaseeOrigId, -1);
+        continue;
----------------
Maybe use a different type of edge for the alias -> aliasee? I.e. another type beyond reference or call with hotness?


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:297
+      defineExternalNode(OS, "  ", getValueInfo(E.Dst));
+      ModList.push_back(-1);
+    }
----------------
might as well add continue at the end of this if body since the subsequent handling is for the non-empty ModList case.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:300
+    for (auto DstMod : ModList)
+      if (DstMod != E.SrcMod)
+        DrawEdge("  ", E.SrcMod, E.Src, DstMod, E.Dst, E.Hotness);
----------------
Since these are cross module edges, when would these be equal?


https://reviews.llvm.org/D41297