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

[Charles Saternos via Phabricator via llvm-commits]

ncharlie added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:613
+  // Calculate the callgraph root
+  FunctionSummary calculateCallGraphRoot() {
+    std::map<ValueInfo, bool> FunctionHasParent;
----------------
tejohnson wrote:
> 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). 
I was having trouble with the ownership of the unique_ptr node generated by this function, so I decided it was simpler to just pass the FunctionSummary directly rather than allocating and managing the memory for it.


================
Comment at: test/ThinLTO/X86/module_summary_graph_traits.ll:32
+define void @l2() {
+  call void @l1()
+  ret void
----------------
tejohnson wrote:
> 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?
I'll look into this.


https://reviews.llvm.org/D36311