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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Just minor comments left at this point, I noted a few places where you hadn't yet addressed my previous commenting nits, plus a question about paring down the new operators, and some suggestions for minorly beefing up the matching you are doing on the test lines.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:320
+
+  static FunctionSummary ExternalNode;
+
----------------
tejohnson wrote:
> document
Not done yet.


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


================
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()))
----------------
tejohnson wrote:
> nit - move comment to preceding line.
Not done yet


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:119
+
+inline bool operator!=(ValueInfo &A, ValueInfo &B) {
+  assert(A.Ref && B.Ref && "Need ValueInfo with non-null Ref to compare GUIDs");
----------------
I see you have both const and non-const parameter versions of operator== and operator<, but only the non-const parameter version of operator!=. Why is that? Also, since ValueInfo::getGUID is const, I would think you should be able to have only a single version of each operator overload, all taking const ValueInfo.


================
Comment at: test/ThinLTO/X86/module_summary_graph_traits.ll:12
+; CHECK: SCC (1 node) {
+; CHECK-NEXT: 15440740835768581517
+; CHECK-NEXT: }
----------------
Add "{{$}}" after each of these that shouldn't be followed by " (has loop)".
Ditto for matching start of line for those that shouldn't be preceeded by "External". I.e. I think preceeding the GUID you are matching with "{{^}} " should do it (match start of line followed by one space which appears to be how these are getting emitted).


================
Comment at: test/ThinLTO/X86/module_summary_graph_traits.ll:24
+; CHECK: SCC (1 node) {
+; CHECK-NEXT: 0
+; CHECK-NEXT: }
----------------
What is this node? Ah, the dummy call graph root? Document.


================
Comment at: test/ThinLTO/X86/module_summary_graph_traits.ll:6
+
+; CHECK: SCC (1 node) {
+; CHECK-NEXT:  tmp1.bc 765152853862302398 (has loop)
----------------
tejohnson wrote:
> Put a comment above each of these as to which function name it corresponds to.
Please add this - I think what you are getting now makes sense, but it would be really helpful to have these documented. BTW, you can check this correlation by passing -print-summary-global-ids to llvm-lto2.


https://reviews.llvm.org/D36311