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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM, with minor comment nits below.



================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4682
+ModuleSummaryIndexBitcodeReader::addThisModulePath() {
+  return TheIndex.addModulePath(ModulePath, ModuleId);
+}
----------------
pcc wrote:
> 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.
I see, ok nevermind for now then.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:710
+  StringRef ModulePath;
+  unsigned ModuleId;
+
----------------
Document


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5487
 
 // Parse the specified bitcode buffer, returning the function info index.
+Error BitcodeModule::readSummary(ModuleSummaryIndex &CombinedIndex,
----------------
This comment goes with getSummary().


https://reviews.llvm.org/D32469