[PATCH] D135714: [MemProf] ThinLTO summary support

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm, thanks for your patience with the slow review.



================
Comment at: llvm/include/llvm/Bitcode/BitcodeReader.h:124-125
+        uint64_t ModuleId,
+        std::function<bool(GlobalValue::GUID)> IsPrevailing =
+            [](GlobalValue::GUID) { return true; });
   };
----------------
Can this go away now that we check before calling the functor?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:6635-6636
+  // the callee function summary is not included in the index. The bitcode
+  // writer records 0. Better way to deal with this so we don't have to disable
+  // this assert for all clients of this method?
+  assert(AllowNullValueInfo || std::get<0>(VGI));
----------------
Is this a TODO?

Also I would write this with template specialization to avoid the need to #ifdef in the decl and the definition.

template<bool AllowNullValueInfo = false>
ModuleSummaryIndexBitcodeReader::getValueInfoFromValueId(unsigned ValueId);

Then the callsite at L7510 can just be getValueInfoFromValueId</*AllowNullValueInfo*/=true> without having a separate function signature in the debug and non-debug builds.





================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:7494
+    case bitc::FS_COMBINED_CALLSITE_INFO: {
+      unsigned ValueID = Record[0];
+      unsigned I = 1;
----------------
I was thinking about using the iterator here, so it would look like the following

```
auto RecordIter = Record.begin();
const unsigned ValueID = *RecordIter;
const unsigned NumStackIds = *RecordIter++;
const unsigned NumVersions = *RecordIter++;
...
```
IMO this pattern is a little better than using an Index variable but I don't have a strong opinion if you would like to keep as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135714