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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 10 inline comments as done.
pcc added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:775
+    }
+    ModuleMap.insert({I.first(), (*BMsOrErr)[0]});
     OwnedImports.push_back(std::move(*MBOrErr));
----------------
mehdi_amini wrote:
> We load the first BitcodeModule, should we assert that this is the only one? How do we know this is the right one?
I added some code that makes sure that we load the right module.


================
Comment at: llvm/lib/LTO/LTO.cpp:212
+    size_t SymBegin = File->SymTab.symbols().size();
+    File->SymTab.addModule(MOrErr->get());
+    size_t SymEnd = File->SymTab.symbols().size();
----------------
mehdi_amini wrote:
> Is there an expectation that no symbol can be duplicated in the various modules?
If you mean that the modules may not both contain a definition of the symbol, then yes. The entity creating these bitcode files is expected to adhere to this policy. (The same applies to definitions of comdats, so the code on lines 215-221 below remains the same.)

It's possible to have a defined symbol in one module and a duplicate undefined symbol with the same name in another module; the situation is similar to module inline asm right now (i.e. PR30396). Whatever solution we come up with for that problem should also apply to this one.


================
Comment at: llvm/lib/LTO/LTO.cpp:250
+  return Mods[0].Mod->getSourceFileName();
+}
+
----------------
mehdi_amini wrote:
> You can create an InputFile where we wouldn't find any module, if you don't want to support that we should detect it in the `create()` and return an error there. Otherwise this is UB to be called on a valid created `InputFile`
> 
> 
Added a check to `create()`.


================
Comment at: llvm/lib/LTO/LTO.cpp:456
 
-  ThinLTO.ModuleMap[MBRef.getBufferIdentifier()] = MBRef;
+  ThinLTO.ModuleMap.insert({BM.getModuleIdentifier(), BM});
   return Error::success();
----------------
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.)


https://reviews.llvm.org/D27313