[PATCH] D135714: [MemProf] ThinLTO summary support

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 15 inline comments as done.
tejohnson added inline comments.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:9570
+
+    std::vector<uint8_t> Versions;
+    do {
----------------
snehasish wrote:
> nit: Given the small number of items to parse using llvm::SmallVector would probably eliminate dynamic allocations?
> 
Ended up changing the type of Versions/Clones/StackIdIndices in the index and throughout.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3906
 
+template <typename Fn1, typename Fn2>
+static void
----------------
snehasish wrote:
> Does this need to be templated? Looks like the uses have the same signature.
> 
> L3974: Fn1 ValueInfo -> unsigned, Fn2 unsigned -> unsigned
> L4434:  Fn1 ValueInfo -> unsigned, Fn2 unsigned -> unsigned
> 
Yeah I don't remember why I templated it in the first place!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135714