[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: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>(),
----------------
tejohnson wrote:
> 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.
Yeah - having a createDummyNode function (or something like that) would definitely help with the readability.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:74
+
+void ModuleSummaryIndex::dumpSCCs(raw_ostream &O) {
+  for (scc_iterator<ModuleSummaryIndex *> I =
----------------
davide wrote:
> Shouldn't this be under `NDEBUG || LLVM_ENABLE_DUMP` (or whatever it's called?)
Won't that remove it in non-debug builds though? That would break the new llvm-lto2 dump-sccs subcommand. That's fine if llvm-lto2 isn't expected to be used except in debug builds.


================
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)) {
----------------
tejohnson wrote:
> 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)
> Maybe add an option to LTO.cpp and optionally dump it out at the start of runThinLTO?

Yeah, I though it was a bit weird having it in here. I'll replace it with a hidden flag in LTO.cpp.


https://reviews.llvm.org/D36311