<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 12, 2018 at 10:45 AM, Eugene Leviant via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">evgeny777 added inline comments.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/IR/ModuleSummaryIndex.cpp:<wbr>275<br>
+<br>
+  for (auto &E : CrossModuleEdges) {<br>
+    auto &ModList = NodeMap[E.Dst];<br>
----------------<br>
</span><span class="">tejohnson wrote:<br>
> 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::<wbr>getModuleId) instead of generating your own, or use the module hash<br>
><br>
> It might be nice to emit the full module path somewhere (maybe as a comment).<br>
> Why are the nodes and edges emitted this way<br>
<br>
</span>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.<br></blockquote><div><br></div><div>Ah, missed the use of clusters. That makes sense (with suggestions below to use existing module id or hash).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> You could just use the module Id stored in the module path string table<br>
</span><span class="">> It might be nice to emit the full module path somewhere (maybe as a comment).<br>
<br>
</span>Makes sense<br>
<br>
<br>
<a href="https://reviews.llvm.org/D41297" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D41297</a><br>
<br>
<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> 408-460-2413</td></tr></tbody></table></span></div>
</div></div>