[PATCH] D135714: [MemProf] ThinLTO summary support

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

Still reading to understand how things work here ...



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:310
+      : Callee(Callee), StackIdIndices(std::move(StackIdIndices)) {
+    Versions.push_back(0);
+  }
----------------
We can specify this in the declaration with an initializer list instead of calling push_back.




================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:318
+
+struct MIBInfo;
+
----------------
Can we reorder AllocInfo and MIBInfo to avoid this forward reference?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:923
+      std::function<bool(GlobalValue::GUID)> IsPrevailing =
+          [](GlobalValue::GUID) { return true; });
 
----------------
Instead of unconditionally returning true, if we modify the call to IsPrevailing and check whether IsPrevailing is empty then the codegen at the callsite is a little better. 

Example - Line L43-L46 in the codegen is eliminated.
https://godbolt.org/z/qfodoc3T9


================
Comment at: llvm/lib/IR/AsmWriter.cpp:3188
     printTypeIdInfo(*TIdInfo);
 
+  auto AllocTypeName = [](uint8_t Type) {
----------------
Can you add a comment here indicating that the AllocationType identifiers capture the profiled context behaviour. Thus "notcoldandcold" implies there are multiple contexts which reach this site, some of which are cold.


================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:424
+  auto *CallsiteSummary =
+      cast<FunctionSummary>(Index->getGlobalValueSummary(25));
+  bool First = true;
----------------
I think this will segfault the test if makeLLVMIndex fails and returns a nullptr.


================
Comment at: llvm/unittests/Analysis/MemoryProfileInfoTest.cpp:439
+
+  auto *AllocSummary = cast<FunctionSummary>(Index->getGlobalValueSummary(23));
+  for (auto &AI : AllocSummary->allocs()) {
----------------
/*guid=*/23 to make it clear what we are referring to.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135714/new/

https://reviews.llvm.org/D135714