[PATCH] D27313: LTO: Add support for multi-module bitcode files.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:783
+      if (HasSummary && *HasSummary) {
+        ModuleMap.insert({I.first(), BM});
+        FoundModule = true;
----------------
What if `I.first` was already present in the map? It seems the same case as below for ThinLTO where we should check that the returned value of `insert().second` .


================
Comment at: llvm/lib/LTO/LTO.cpp:456
 
-  ThinLTO.ModuleMap[MBRef.getBufferIdentifier()] = MBRef;
+  ThinLTO.ModuleMap.insert({BM.getModuleIdentifier(), BM});
   return Error::success();
----------------
pcc wrote:
> mehdi_amini wrote:
> > We'll arrive here from ` for (InputFile::InputModule &IM : Input->Mods)` in `LTO::add()`, how are we distinguishing `BM.getModuleIdentifier()` when there are multiple modules per file?
> > I assume we don't expect multiple modules with a summary in the same input file, but in this case we should check here that the module has been inserted and error otherwise.
> `BM.getModuleIdentifier()` will be the same for each `BitcodeModule` in the same input file by construction, so we can just test the return value of `insert` here.
> 
> (I feel obliged to point out that this is yet another scenario where (path, byte slice) pairs would work better as a representation of ThinLTO modules.)
What is the other scenario? I don't remember what was specific to ThinLTO for this?

Also a pair `path/byte slice` doesn't fit the API in general, does it? We don't necessarily have a "file".
That said, instead of path the identifier could be a `<BufferID, offset>`, with `BufferID` intended to be a unique identifier provided by the client of the API per-buffer. I'm not sure how deep we'd have to thread this though. I think that as long as when we create a llvm::Module the identifier is consistent with whatever is used here, everything should be OK.



https://reviews.llvm.org/D27313