[PATCH] D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:760
+using TypeIdSummaryMapTy =
+    std::map<GlobalValue::GUID,
+             std::vector<std::pair<std::string, TypeIdSummary>>>;
----------------
vitalybuka wrote:
> Have you considered multimap?
> 
> ```
> using TypeIdSummaryMapTy =
>     std::multimap<GlobalValue::GUID, std::pair<std::string, TypeIdSummary>>;
> ```
> 
> Not sure, but maybe code could be a little bit simpler with it.
> 
> 
Changed the code to use multimap. 


================
Comment at: lib/IR/AsmWriter.cpp:2926
+      // Print all type id that correspond to this GUID.
+      for (auto &Tid : I->second) {
+        Out << FS;
----------------
vitalybuka wrote:
> So now we can have multiple entries here. Would be nice to reach this with a test?
This requires identifying two strings that result in an MD5 collision, which should be extremely rare. I found an example on the web but it is in hex and does not convert to a valid string. So I'm not really sure how to test this.


================
Comment at: lib/IR/AsmWriter.cpp:2969
+    Out << FS;
+    Out << "vFuncId: (";
+    auto Slot = Machine.getTypeIdSlot(Tid.first);
----------------
vitalybuka wrote:
> Test?
Ditto.


Repository:
  rL LLVM

https://reviews.llvm.org/D51330