[PATCH] D36311: [ThinLTO] Add GraphTraits for FunctionSummaries

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

This looks really good, just a few minor comments. But I do have a concern about the test and it not showing the multiple node SCCs that I would expect. Can you investigate?



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:320
+
+  static FunctionSummary ExternalNode;
+
----------------
document


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:613
+  // Calculate the callgraph root
+  FunctionSummary calculateCallGraphRoot() {
+    std::map<ValueInfo, bool> FunctionHasParent;
----------------
Notice this is no longer lazily calculated. Was there a specific reason for the change? I suppose one reason not to do that is that you'd want any changes to the Index reflected in this in between subsequent SCC graph constructions (which should not happen frequently anyway). 


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:628
+        auto S = FunctionHasParent.find(C.first);
+        if (S != FunctionHasParent.end() && S->second)
+          continue;
----------------
comment


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:636
+    for (auto &S : *this) {
+      if (!S.second.SummaryList.size() || // skip external functions
+          !isa<FunctionSummary>(S.second.SummaryList.front().get()))
----------------
nit - move comment to preceding line.


================
Comment at: test/ThinLTO/X86/module_summary_graph_traits.ll:6
+
+; CHECK: SCC (1 node) {
+; CHECK-NEXT:  tmp1.bc 765152853862302398 (has loop)
----------------
Put a comment above each of these as to which function name it corresponds to.


================
Comment at: test/ThinLTO/X86/module_summary_graph_traits.ll:32
+define void @l2() {
+  call void @l1()
+  ret void
----------------
ncharlie wrote:
> Despite the indirect recursion in l1/l2, the SCCs only have 1 node in each. The loop is still detected (with the scc_iterator::hasLoop function), but there's an SCC for each function.
Hmm, that seems wrong? Wouldn't we incorrectly think these were NoRecurse then? Wondering also why there are 3 functions marked as "has loop". I assume that is root() - but we haven't done any inlining that would cause this right?


https://reviews.llvm.org/D36311