[PATCH] D41297: [ThinLTO] Implement summary visualizer

[Teresa Johnson via llvm-commits]

On Fri, Jan 12, 2018 at 10:45 AM, Eugene Leviant via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>

Ah, missed the use of clusters. That makes sense (with suggestions below to
use existing module id or hash).


> > 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
>
>
>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180112/5713e01c/attachment.html>