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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Getting there, thanks!



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:436-444
+            GlobalValue::LinkageTypes::AvailableExternallyLinkage, true, false),
+        0, FunctionSummary::FFlags{}, std::vector<ValueInfo>(),
+        std::vector<FunctionSummary::EdgeTy>(),
+        std::vector<GlobalValue::GUID>(),
+        std::vector<FunctionSummary::VFuncId>(),
+        std::vector<FunctionSummary::VFuncId>(),
+        std::vector<FunctionSummary::ConstVCall>(),
----------------
davide wrote:
> This is not really readable. I wonder if you can make it shorter.
Not sure how to make the constructor invocation shorter, but perhaps the code that invokes make_unique could be extracted out to a helper and invoked both here and below where the dummy call graph root node is being constructed. It only needs to take a vector of edges since that is the only difference AFAICT between the 2 invocations.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:649
+            FunctionHasParent.find(C.first) == FunctionHasParent.end();
+        if (notDiscovered) {
+          FunctionHasParent[C.first] = true;
----------------
Simplify the above by simply doing:

if (FunctionHasParent.find(C.first) != FunctionHasParent.end())
  continue;

Actually - I don't think this is quite correct. We would have emplaced an entry for a function (with bool of false) if we encountered it first through the index walk in the loop below this. So I think you are going to need to assign the result of find() to a variable and then continue if it is != end() and is set to true.

Not sure if this fix is testable - I suppose you were ending up with more nodes than necessary connected to the CallGraphRoot, but I'm not sure offhand what the implication of that is.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:657
+    for (auto &S : *this) {
+      if (!S.second.SummaryList.size() ||
+          !isa<FunctionSummary>(S.second.SummaryList.front().get()))
----------------
Needs comment (I can't remember offhand why we would have an index entry with an empty SummaryList - oh, declarations right? I don't think those will show up in the combined index but still ok here but needs to be documented).


================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:396
 
+static int dumpSCCs(int argc, char **argv) {
+  for (StringRef F : make_range(argv + 1, argv + argc)) {
----------------
I'm not sure llvm-lto2 is the right place for this. Specifically because we normally want to dump these on the combined index that is usually in-memory. Maybe add an option to LTO.cpp and optionally dump it out at the start of runThinLTO?

(Note also this appears to be built on top of your llvm-lto2 with the dump-summary change, which unfortunately we have not gotten agreement to commit yet)


https://reviews.llvm.org/D36311