[PATCH] D54815: [ThinLTO] Add summary entries for index-based WPD

[Teresa Johnson via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:896
+  /// above where TypeIdCompatibleVtableInfo is defined.
+  std::map<std::string, TypeIdCompatibleVtableInfo> TypeIdCompatibleVtableMap;
+
----------------
Prazek wrote:
> Do we care about the alphabetical order in this mapping?
> If not, the map can be changed to std::unordered_map (or other llvm-specific string map), which should be faster and more memory efficient.
We care about consistent ordering, as the iteration order will affect the order of the summary entries emitted into the bitcode. So unordered_map is not appropriate here. Ditto for StringMap.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3691
 
-  Stream.EmitRecord(bitc::FS_PERMODULE_GLOBALVAR_INIT_REFS, NameVals,
-                    FSModRefsAbbrev);
+  if (!VTableFuncs.empty()) {
+    // VTableFuncs pairs should already be sorted by offset.
----------------
Prazek wrote:
> Isn't this line redundant?
yep, will fix.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3857
 
+  if (!Index->typeIdCompatibleVtableMap().empty()) {
+    for (auto &S : Index->typeIdCompatibleVtableMap()) {
----------------
Prazek wrote:
> Isn't this line redundant?
yep will fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54815