[PATCH] D135714: [MemProf] ThinLTO summary support

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/include/llvm/AsmParser/LLToken.h:397
   kw_varFlags,
+  kw_callsites,
+  kw_versions,
----------------
Should we add a comment indicating these are used by memprof?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:300
+  // original callee.
+  std::vector<unsigned> Versions;
+
----------------
Call this Clones instead of Versions to avoid confusion with AllocType Version? Also see the related comment below about introducing a separate kw_clones token.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:1335
+  unsigned addOrGetStackIdIndex(uint64_t StackId) {
+    if (StackIdToIndex.count(StackId))
+      return StackIdToIndex[StackId];
----------------
We can replace 2 lookups in the map with just one if we use `insert` and check the boolean indicating whether an insertion happened?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:1355
+    assert(StackIdToIndex.size() == StackIds.size());
+    StackIdToIndex.clear();
+  }
----------------
Maybe call `StackIds.shrink_to_fit()` as well to trim the vector too?


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:9570
+
+    std::vector<uint8_t> Versions;
+    do {
----------------
nit: Given the small number of items to parse using llvm::SmallVector would probably eliminate dynamic allocations?



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:9707
+    if (parseToken(lltok::comma, "expected ',' in callsite") ||
+        parseToken(lltok::kw_versions, "expected 'versions' in callsite") ||
+        parseToken(lltok::colon, "expected ':'") ||
----------------
It would be great to use a separate token such as kw_clones here instead of reusing version for clarity. It caused a bit of confusion for me when reading the bitcode in the test.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:7486
+      std::vector<unsigned> StackIdList;
+      while (I < Record.size()) {
+        assert(Record[I] < StackIds.size());
----------------
Rewriting this as `for(unsigned I = 1; I < Record.size; I++)` would reduce the scope of I a bit more and make it readable overall I think.

Alternatively consider using an iterator for Record so we don't have to worry about indices? This might be a better approach for the code below.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3906
 
+template <typename Fn1, typename Fn2>
+static void
----------------
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



================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3912
+                                Fn1 GetValueID, Fn2 GetStackIndex) {
+  SmallVector<uint64_t, 64> Record;
+
----------------
I think we can let the SmallVector infer the number of elements automatically unless we have a good idea of how many records we might have.

https://llvm.org/doxygen/classllvm_1_1SmallVector.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135714