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

[Peter Collingbourne via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4682
+ModuleSummaryIndexBitcodeReader::addThisModulePath() {
+  return TheIndex.addModulePath(ModulePath, ModuleId);
+}
----------------
tejohnson wrote:
> 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().
The problem is that we don't know until we start parsing the file whether it is a per-module summary or a combined summary, so we could end up adding an entry for the combined summary file in the list of module paths. I guess we could avoid this problem by changing the API so that `getSummary` is only used for combined summaries and `readSummary` for per-module summaries, but that can probably be done separately.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4997
-      if (!Combined)
-        TheIndex.removeEmptySummaryEntries();
       return Error::success();
----------------
tejohnson wrote:
> 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?
r301660.


================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:573
+    if (Error Err = readModuleSummaryIndex(ModuleBuffer.getMemBuffer(),
+                                           *CombinedIndex, ++NextModuleId)) {
       // FIXME diagnose
----------------
tejohnson wrote:
> 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.
I don't think it matters, but it could change the contents of the combined summary bitcode file and this change is supposed to be NFC, so I've changed it back.


https://reviews.llvm.org/D32469