[PATCH] D32469: Bitcode: Make the summary reader responsible for merging. NFCI.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4682
+ModuleSummaryIndexBitcodeReader::addThisModulePath() {
+  return TheIndex.addModulePath(ModulePath, ModuleId);
+}
----------------
Can we instead add the module path in the constructor, then I think there is no need to save the ModuleId in the class? And then this can be changed to something like getThisModulePath and have it return the StringRef. In the one place where this was being called to get the Hash reference, you could then instead invoke the existing getModuleHash().


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4997
-      if (!Combined)
-        TheIndex.removeEmptySummaryEntries();
       return Error::success();
----------------
Actually, I believe the need for this went away with D19462, which reorganized the data structures and the way that we built the index during summary parsing so that we no longer created the summary entries eagerly during VST parsing. In fact, the stated need for it here is even more true with this patch (that we merge everything into the first module), so this patch wouldn't have removed the need for this if we hadn't already removed it. Can you commit the change to remove this separately?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5503
+Expected<std::unique_ptr<ModuleSummaryIndex>>
+BitcodeModule::getSummary() {
   BitstreamCursor Stream(Buffer);
----------------
Spurious new line added here? No other change to the original line.


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:573
+    if (Error Err = readModuleSummaryIndex(ModuleBuffer.getMemBuffer(),
+                                           *CombinedIndex, ++NextModuleId)) {
       // FIXME diagnose
----------------
Previously the first module's index would get module ID 0 (since we simply used the first parsed module index as the combined index). But I don't think this has any meaningful impact, right? Anyway, it is more consistent with llvm-lto which will also start the numbering at 1.


https://reviews.llvm.org/D32469