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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:445
+
+  // Use the first callee as the entry node
+  static NodeRef getEntryNode(FunctionSummary *F) { return F; }
----------------
This isn't the first callee, it is the function summary whose callees we will walk as children.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:606
 
+  std::unique_ptr<FunctionSummary> CallGraphRoot;
+
----------------
doxygen comment needed


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:646
+            FunctionHasParent[*I] = true;
+            if (!isDiscovered(*I))
+              discoverNodes(*I);
----------------
Since you set FunctionHasParent on the line above, I think this would always already return true for isDiscovered.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:662
+    for (auto &P : FunctionHasParent) {
+      if (!P.second) {
+        ValueInfo V = getOrInsertValueInfo(P.first->getOriginalName());
----------------
Better to reverse sense of condition and do continue.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:663
+      if (!P.second) {
+        ValueInfo V = getOrInsertValueInfo(P.first->getOriginalName());
+        assert(V.Ref && V.getSummaryList().size());
----------------
Why are you using original name here? I guess it is because you don't have easy access to the normal GUID? Can you fix this by changing FunctionHasParent to key off the ValueInfo instead of the FunctionSummary? I.e. then in the above loop over the index you would call discoverNodes with ValueInfo(&S). Then here you have direct access to the ValueInfo.

I guess the type of NodeRef and return type of fsumFromEdge need to change to ValueInfo correspondingly.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:671
+        FunctionSummary::GVFlags(
+            GlobalValue::LinkageTypes::AvailableExternallyLinkage, true, false),
+        0, FunctionSummary::FFlags{}, std::vector<ValueInfo>(),
----------------
Add a comment describing the parameters you are passing constant values. E.g. "/*NotEligibleToImport=*/true".


https://reviews.llvm.org/D36311