[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:771
+      handleAllErrors(BMsOrErr.takeError(), [&](ErrorInfoBase &EIB) {
+        errs() << "Error running ThinLTO backend: " << EIB.message() << '\n';
+      });
----------------
The error message could be a bit more specific here.


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


================
Comment at: llvm/include/llvm/LTO/LTO.h:224
+  StringRef getName() const;
+  StringRef getSourceFileName() const;
 
----------------
Since we have the two, it seems worth documenting these.


================
Comment at: llvm/include/llvm/LTO/LTO.h:421
+                   iterator_range<InputFile::symbol_iterator> Syms,
+                   const SymbolResolution *&ResI, const SymbolResolution *ResE);
 
----------------
This is a pretty annoying API with ResI as an InOut parameter: at least document it.


================
Comment at: llvm/lib/LTO/LTO.cpp:187
+  std::unique_ptr<Module> Mod;
+  size_t SymBegin, SymEnd;
+};
----------------
Document.


================
Comment at: llvm/lib/LTO/LTO.cpp:190
+
+InputFile::~InputFile() = default;
+
----------------
Add a comment to explain why it is out-of-line.


================
Comment at: llvm/lib/LTO/LTO.cpp:200
 
-  File->Mod = std::move(*MOrErr);
-  File->SymTab.addModule(File->Mod.get());
+  Expected<std::vector<BitcodeModule>> BMsOrErr =
+      getBitcodeModuleList(*BCOrErr);
----------------
Some high level one or two sentences commenting what is going to happen below in the loop would be welcome I think.


================
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();
----------------
Is there an expectation that no symbol can be duplicated in the various modules?


================
Comment at: llvm/lib/LTO/LTO.cpp:250
+  return Mods[0].Mod->getSourceFileName();
+}
+
----------------
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`




================
Comment at: llvm/lib/LTO/LTO.cpp:276
 
+LTO::~LTO() = default;
+
----------------
Add a comment to explain why it is out-of-line.


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


https://reviews.llvm.org/D27313