[PATCH] D29067: IRGen: When loading the main module in the distributed ThinLTO backend, look for the module containing the summary.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: clang/include/clang/CodeGen/BackendUtil.h:51
+  llvm::Expected<llvm::BitcodeModule>
+  FindThinLTOModule(llvm::MemoryBufferRef MBRef);
 }
----------------
mehdi_amini wrote:
> Indentation seems strange?
Yes, it's what clang-format does though.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:867
 
+Expected<BitcodeModule> clang::FindThinLTOModule(MemoryBufferRef MBRef) {
+  Expected<std::vector<BitcodeModule>> BMsOrErr = getBitcodeModuleList(MBRef);
----------------
tejohnson wrote:
> Would it be better to have this in llvm (e.g. in BitcodeReader like getBitcodeModuleList), or do you anticipate that we will never need that functionality within llvm itself?
Maybe -- at this point I feel that we are shoehorning far too much thinlto backend functionality into clang and that we should think about making it into a separate tool. If we ever create such a tool, we can easily move this function there.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:841
 
+std::unique_ptr<llvm::Module> CodeGenAction::loadModule(MemoryBufferRef MBRef) {
+  CompilerInstance &CI = getCompilerInstance();
----------------
mehdi_amini wrote:
> Can you extract this function in a separate patch? I feel the diff could be a lot more contained here.
> 
Done for this function as well as loadModule in r292970 and r292972.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:848
+  // the correct module out of a multi-module bitcode file.
+  if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty()) {
+    VMContext->enableDebugTypeODRUniquing();
----------------
tejohnson wrote:
> What should happen if we have a multi-module bitcode file but no ThinLTOIndexFile? Right now I think that would error (with or without this patch), right? Do we instead want to compile the non-ThinLTO module, or compile the ThinLTO module without ThinLTO (which is what would happen if you passed a single-module bitcode file to clang without -fthinlto-index).
Yes, we'd error out in that case, and I think that's fine. Basically at the moment we only support passing ThinLTO output to either the linker or the thin backend, and anything else is unsupported.


================
Comment at: clang/test/CodeGen/thinlto_backend.ll:26
+; RUN: llvm-cat -b -o %t5.o %t1.o %t2merge.o
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t6.o -x ir %t5.o -c -fthinlto-index=%t.empty.thinlto.bc -mllvm -ignore-empty-index-file
+; RUN: llvm-nm %t5.o | FileCheck --check-prefix=CHECK-OBJ-IGNORE-EMPTY %s
----------------
tejohnson wrote:
> Are we testing the right thing here? How do we know we have loaded the correct module when passed an empty index file which causes us to drop to non-ThinLTO compilation with -ignore-empty-index-file? Maybe that handling is later, but it isn't clear from this test - can you instead test using an index file that enables ThinLTO compilation?
Done, and split out into a separate test.


https://reviews.llvm.org/D29067