[PATCH] D135714: [MemProf] ThinLTO summary support

[Teresa Johnson via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Bitcode/BitcodeReader.h:124-125
+        uint64_t ModuleId,
+        std::function<bool(GlobalValue::GUID)> IsPrevailing =
+            [](GlobalValue::GUID) { return true; });
   };
----------------
snehasish wrote:
> Can this go away now that we check before calling the functor?
Yes, we can make it default to nullptr as we do in the ModuleSummaryIndexBitcodeReader constructor.


================
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));
----------------
snehasish wrote:
> 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.
> 
> 
> 
That was a stale note to self from an older version that didn't pass in AllowNullValueInfo and simply disabled the assert, that I forgot to clean up.

Good idea on the template! In fact, the callsite was incorrect as it was always passing the additional parameter without checking for build mode, and would have failed release builds...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135714