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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM with a few suggestions below that don't require a re-review. Minor update needed to the ValueInfo comparisons due to a recent patch (see the comment I left there), and a few efficiency improvements. Go ahead and submit once those are fixed and tests pass and you've uploaded the new patch here for posterity.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:109
 
-static bool operator==(ValueInfo A, ValueInfo B) {
-  assert(A.Ref && B.Ref);
+inline bool operator==(const ValueInfo &A, const ValueInfo &B) {
+  assert(A.Ref && B.Ref && "Need ValueInfo with non-null Ref to compare GUIDs");
----------------
The asserts in these comparison operators will need a slight update after the changes to ValueInfo in r323062. I think you can just change Ref to getRef() on the given ValueInfo objects.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:625
   FunctionSummary calculateCallGraphRoot() {
     std::map<ValueInfo, bool> FunctionHasParent;
     std::function<void(ValueInfo)> discoverNodes = [&](ValueInfo V) {
----------------
Add a comment above this documenting what the bool represents.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:631
       // Mark discovered if we haven't yet
       FunctionHasParent.emplace(std::make_pair(V, false));
 
----------------
Presumably more efficient to check if there is already a map entry and return early if so (we'll eventually skip all the callees below since they would already have been visited, but might as well not even do that processing). Also, no need for make_pair when using emplace. I.e. change this to:

auto S = FunctionHasParent.emplace(V, false);
// Stop if we have already discovered this node.
if (!S.second)
   return;


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:643
         FunctionHasParent[C.first] = true;
         discoverNodes(C.first);
       }
----------------
We can actually skip the discoverNodes if it was in the map (even if S->second was false), since we would already have walked it's callees if it was added to the map when encountered in the for loop below over the index. Additionally, no need to do another map lookup to set the value to true, since you already have the entry via the iterator, although even better to just do via emplace. So you should be able to change this block to:

// Insert the node if necessary
auto S = FunctionHasParent.emplace(C.first, true);
// Skip nodes that already had parents
if (!S.second && S.first->second)
   continue;
// If we just inserted this node, discover it, otherwise just note that it now has a parent.
if (S.second)
   discoverNodes(C.first);
else
   S.first->second = true;

I think this should work...


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:652
         continue;
       discoverNodes(getValueInfo(S.first));
     }
----------------
getValueInfo will lookup the given GUID in the GlobalValueMap - the same map we are iterating over in this loop. Can't we just do "discoverNodes(S.second)"?


https://reviews.llvm.org/D36311