[PATCH] D19556: [ThinLTO] Emit individual index files for distributed backends

[Mehdi AMINI via llvm-commits]

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

LGTM (see one bikeshed suggestion inline)


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:286
@@ +285,3 @@
+      : BitcodeWriter(Buffer), Index(Index),
+        ModuleToSummariesForIndex(ModuleToSummariesForIndex) {
+    // Assign unique value ids to all summaries to be written, for use
----------------
> The ImportMapTy is a map to FunctionsToImportTy, which provides the GUID of the functions to import. I would then need to perform an extra step in the bitcode writer to find the corresponding summary object for that module. It was more natural and efficient to get that directly from the ModuleToDefinedGVSummaries map already populated in the client (and in fact this map and ModuleToDefinedGVSummaries use the same GVSummaryMapTy).

OK I missed this the first time, seems obvious afterward...
It seems like something that could be changed in whatever `ComputeCrossModuleImportForModule` produces, but we can refactor/change that later (indeed this is the kind of information I was expected to gather from your current series of patches to feed the design a common API for linkers).



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:423
@@ +422,3 @@
+/// \p ModulePath.
+void llvm::ComputeSummariesForFile(
+    StringRef ModulePath,
----------------
bikeshed: what about `llvm::gatherImportedSummariesForModule()` (i.e. we don't really "compute", and "File" seems odd compare to the usual use of "Module")


http://reviews.llvm.org/D19556