[PATCH] D41297: [ThinLTO] Implement summary visualizer

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:275
+
+  for (auto &E : CrossModuleEdges) {
+    auto &ModList = NodeMap[E.Dst];
----------------
tejohnson wrote:
> Why are the nodes and edges emitted this way - i.e. intra-module first, then cross-module? Why not just want the index and emit all the edges in one sweep? That would certainly be less expensive. You could just use the module Id stored in the module path string table (see ModuleSummaryIndex::getModuleId) instead of generating your own, or use the module hash
> 
> It might be nice to emit the full module path somewhere (maybe as a comment).
> Why are the nodes and edges emitted this way

This is due to GraphViz specifics. When you define clusters (subgraphs), you should list intra-cluster edges inside cluster definitions, otherwise GraphViz won't display them correctly. So you have two options: (a) do not use clusters and list edges in any order or (b) use clusters and define intra-cluster nodes and edges within cluster definition and cross cluster edges outside of it.

> You could just use the module Id stored in the module path string table
> It might be nice to emit the full module path somewhere (maybe as a comment).

Makes sense


https://reviews.llvm.org/D41297