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

[mehdi_amini]

clang/include/clang/CodeGen/BackendUtil.h

51

Indentation seems strange?

clang/lib/CodeGen/CodeGenAction.cpp

841

Can you extract this function in a separate patch? I feel the diff could be a lot more contained here.

[tejohnson]

clang/lib/CodeGen/BackendUtil.cpp

867

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?

clang/lib/CodeGen/CodeGenAction.cpp

848

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).

clang/test/CodeGen/thinlto_backend.ll

26

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?

[pcc]

clang/include/clang/CodeGen/BackendUtil.h

51

Yes, it's what clang-format does though.

clang/lib/CodeGen/BackendUtil.cpp

867

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.

clang/lib/CodeGen/CodeGenAction.cpp

841

Done for this function as well as loadModule in r292970 and r292972.

848

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.

clang/test/CodeGen/thinlto_backend.ll

26

Done, and split out into a separate test.

[pcc]

Address review comments

[tejohnson]

LGTM

clang/include/clang/CodeGen/BackendUtil.h

 #ifndef LLVM_CLANG_CODEGEN_BACKENDUTIL_H
 #define LLVM_CLANG_CODEGEN_BACKENDUTIL_H
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/IR/ModuleSummaryIndex.h"
 #include <memory>
 
 namespace llvm {
+  class BitcodeModule;
+  template <typename T> class Expected;
   class Module;
   class MemoryBufferRef;
 }
 
 namespace clang {
   class DiagnosticsEngine;
   class HeaderSearchOptions;
   class CodeGenOptions;
void EmitBackendOutput(DiagnosticsEngine &Diags, const HeaderSearchOptions &,
                          const CodeGenOptions &CGOpts,
                          const TargetOptions &TOpts, const LangOptions &LOpts,
                          const llvm::DataLayout &TDesc, llvm::Module *M,
                          BackendAction Action,
                          std::unique_ptr<raw_pwrite_stream> OS);
 
   void EmbedBitcode(llvm::Module *M, const CodeGenOptions &CGOpts,
                     llvm::MemoryBufferRef Buf);
+
+  llvm::Expected<llvm::BitcodeModule>
+  FindThinLTOModule(llvm::MemoryBufferRef MBRef);
 }
 
 #endif

clang/include/clang/CodeGen/CodeGenAction.h

private:
   unsigned Act;
   std::unique_ptr<llvm::Module> TheModule;
   // Vector of {Linker::Flags, Module*} pairs to specify bitcode
   // modules to link in using corresponding linker flags.
   SmallVector<std::pair<unsigned, llvm::Module *>, 4> LinkModules;
   llvm::LLVMContext *VMContext;
   bool OwnsVMContext;
 
+  std::unique_ptr<llvm::Module> loadModule(llvm::MemoryBufferRef MBRef);
+
 protected:
   /// Create a new code generation action.  If the optional \p _VMContext
   /// parameter is supplied, the action uses it without taking ownership,
   /// otherwise it creates a fresh LLVM context and takes ownership.
   CodeGenAction(unsigned _Act, llvm::LLVMContext *_VMContext = nullptr);
 
   bool hasIRSupport() const override;
 

clang/lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
 
   // Now if needed, run the legacy PM for codegen.
   if (NeedCodeGen) {
     PrettyStackTraceString CrashInfo("Code generation");
     CodeGenPasses.run(*TheModule);
   }
 }
 
+Expected<BitcodeModule> clang::FindThinLTOModule(MemoryBufferRef MBRef) {
+  Expected<std::vector<BitcodeModule>> BMsOrErr = getBitcodeModuleList(MBRef);
+  if (!BMsOrErr)
+    return BMsOrErr.takeError();
+
+  // The bitcode file may contain multiple modules, we want the one with a
+  // summary.
+  for (BitcodeModule &BM : *BMsOrErr) {
+    Expected<bool> HasSummary = BM.hasSummary();
+    if (HasSummary && *HasSummary)
+      return BM;
+  }
+
+  return make_error<StringError>("Could not find module summary",
+                                 inconvertibleErrorCode());
+}
+
 static void runThinLTOBackend(ModuleSummaryIndex *CombinedIndex, Module *M,
                               std::unique_ptr<raw_pwrite_stream> OS,
                               std::string SampleProfile) {
   StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>>
       ModuleToDefinedGVSummaries;
   CombinedIndex->collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // We can simply import the values mentioned in the combined index, since
for (auto &I : ImportList) {
     ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MBOrErr =
         llvm::MemoryBuffer::getFile(I.first());
     if (!MBOrErr) {
       errs() << "Error loading imported file '" << I.first()
              << "': " << MBOrErr.getError().message() << "\n";
       return;
     }
 
-    Expected<std::vector<BitcodeModule>> BMsOrErr =
-        getBitcodeModuleList(**MBOrErr);
-    if (!BMsOrErr) {
-      handleAllErrors(BMsOrErr.takeError(), [&](ErrorInfoBase &EIB) {
+    Expected<BitcodeModule> BMOrErr = FindThinLTOModule(**MBOrErr);
+    if (!BMOrErr) {
+      handleAllErrors(BMOrErr.takeError(), [&](ErrorInfoBase &EIB) {
         errs() << "Error loading imported file '" << I.first()
                << "': " << EIB.message() << '\n';
       });
       return;
     }
-
-    // The bitcode file may contain multiple modules, we want the one with a
-    // summary.
-    bool FoundModule = false;
-    for (BitcodeModule &BM : *BMsOrErr) {
-      Expected<bool> HasSummary = BM.hasSummary();
-      if (HasSummary && *HasSummary) {
-        ModuleMap.insert({I.first(), BM});
-        FoundModule = true;
-        break;
-      }
-    }
-    if (!FoundModule) {
-      errs() << "Error loading imported file '" << I.first()
-             << "': Could not find module summary\n";
-      return;
-    }
+    ModuleMap.insert({I.first(), *BMOrErr});
 
     OwnedImports.push_back(std::move(*MBOrErr));
   }
   auto AddStream = [&](size_t Task) {
     return llvm::make_unique<lto::NativeObjectStream>(std::move(OS));
   };
   lto::Config Conf;
   Conf.SampleProfile = SampleProfile;

clang/lib/CodeGen/CodeGenAction.cpp

static void BitcodeInlineAsmDiagHandler(const llvm::SMDiagnostic &SM,
   case llvm::SourceMgr::DK_Note:
     DiagID = diag::note_fe_inline_asm;
     break;
   }
 
   Diags->Report(DiagID).AddString("cannot compile inline asm");
 }
 
-void CodeGenAction::ExecuteAction() {
-  // If this is an IR file, we have to treat it specially.
-  if (getCurrentFileKind() == IK_LLVM_IR) {
-    BackendAction BA = static_cast<BackendAction>(Act);
-    CompilerInstance &CI = getCompilerInstance();
-    std::unique_ptr<raw_pwrite_stream> OS =
-        GetOutputStream(CI, getCurrentFile(), BA);
-    if (BA != Backend_EmitNothing && !OS)
-      return;
-
-    bool Invalid;
-    SourceManager &SM = CI.getSourceManager();
-    FileID FID = SM.getMainFileID();
-    llvm::MemoryBuffer *MainFile = SM.getBuffer(FID, &Invalid);
-    if (Invalid)
-      return;
+std::unique_ptr<llvm::Module> CodeGenAction::loadModule(MemoryBufferRef MBRef) {
 
-    // For ThinLTO backend invocations, ensure that the context
-    // merges types based on ODR identifiers.
-    if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty())
-      VMContext->enableDebugTypeODRUniquing();
+  // merges types based on ODR identifiers. We also need to read
+  // the correct module out of a multi-module bitcode file.
+  if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty()) {
 
-    llvm::SMDiagnostic Err;
-    TheModule = parseIR(MainFile->getMemBufferRef(), Err, *VMContext);
-    if (!TheModule) {
-      // Translate from the diagnostic info to the SourceManager location if
-      // available.
-      // TODO: Unify this with ConvertBackendLocation()
-      SourceLocation Loc;
-      if (Err.getLineNo() > 0) {
-        assert(Err.getColumnNo() >= 0);
-        Loc = SM.translateFileLineCol(SM.getFileEntryForID(FID),
-                                      Err.getLineNo(), Err.getColumnNo() + 1);
-      }
+    auto DiagErrors = [&](Error E) -> std::unique_ptr<llvm::Module> {
+      unsigned DiagID =
+          CI.getDiagnostics().getCustomDiagID(DiagnosticsEngine::Error, "%0");
+      handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
+        CI.getDiagnostics().Report(DiagID) << EIB.message();
+      });
+      return {};
+    };
+
+    Expected<llvm::BitcodeModule> BMOrErr = FindThinLTOModule(MBRef);
+    if (!BMOrErr)
+      return DiagErrors(BMOrErr.takeError());
+
+    Expected<std::unique_ptr<llvm::Module>> MOrErr =
+        BMOrErr->parseModule(*VMContext);
+    if (!MOrErr)
+      return DiagErrors(MOrErr.takeError());
+    return std::move(*MOrErr);
+  }
+
+  if (std::unique_ptr<llvm::Module> M = parseIR(MBRef, Err, *VMContext))
+    return M;
+
+    Loc = SM.translateFileLineCol(SM.getFileEntryForID(SM.getMainFileID()),
 
-      // Strip off a leading diagnostic code if there is one.
-      StringRef Msg = Err.getMessage();
-      if (Msg.startswith("error: "))
-        Msg = Msg.substr(7);
 
-      unsigned DiagID =
-          CI.getDiagnostics().getCustomDiagID(DiagnosticsEngine::Error, "%0");
 
-      CI.getDiagnostics().Report(Loc, DiagID) << Msg;
-      return;
-    }
+  return {};
+
+void CodeGenAction::ExecuteAction() {
+  // If this is an IR file, we have to treat it specially.
+  if (getCurrentFileKind() == IK_LLVM_IR) {
+    BackendAction BA = static_cast<BackendAction>(Act);
+    CompilerInstance &CI = getCompilerInstance();
+    std::unique_ptr<raw_pwrite_stream> OS =
+        GetOutputStream(CI, getCurrentFile(), BA);
+    if (BA != Backend_EmitNothing && !OS)
+      return;
+
+    bool Invalid;
+    SourceManager &SM = CI.getSourceManager();
+    FileID FID = SM.getMainFileID();
+    llvm::MemoryBuffer *MainFile = SM.getBuffer(FID, &Invalid);
+    if (Invalid)
+      return;
+
+    TheModule = loadModule(*MainFile);
+    if (!TheModule)
+      return;
+
     const TargetOptions &TargetOpts = CI.getTargetOpts();
     if (TheModule->getTargetTriple() != TargetOpts.Triple) {
       CI.getDiagnostics().Report(SourceLocation(),
                                  diag::warn_fe_override_module)
           << TargetOpts.Triple;
       TheModule->setTargetTriple(TargetOpts.Triple);
     }
 

clang/test/CMakeLists.txt

set(CLANG_TEST_PARAMS
   )
 
 if( NOT CLANG_BUILT_STANDALONE )
   list(APPEND CLANG_TEST_DEPS
     llvm-config
     FileCheck count not
     llc
     llvm-bcanalyzer
+    llvm-cat
     llvm-dis
     llvm-modextract
     llvm-nm
     llvm-objdump
     llvm-profdata
     llvm-readobj
     llvm-symbolizer
     opt

clang/test/CodeGen/thinlto_backend.ll

 ; REQUIRES: x86-registered-target
 
 ; RUN: opt -module-summary -o %t1.o %s
 ; RUN: opt -module-summary -o %t2.o %S/Inputs/thinlto_backend.ll
+; RUN: opt -o %t2merge.o %S/Inputs/thinlto_backend.ll
 ; RUN: llvm-lto -thinlto -o %t %t1.o %t2.o
 
 ; Ensure clang -cc1 give expected error for incorrect input type
 ; RUN: not %clang_cc1 -O2 -o %t1.o -x c %s -c -fthinlto-index=%t.thinlto.bc 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
 ; CHECK-WARNING: error: invalid argument '-fthinlto-index={{.*}}' only allowed with '-x ir'
 
 ; Ensure we get expected error for missing index file
 ; RUN: %clang -O2 -o %t4.o -x ir %t1.o -c -fthinlto-index=bad.thinlto.bc 2>&1 | FileCheck %s -check-prefix=CHECK-ERROR1
 ; CHECK-ERROR1: Error loading index file 'bad.thinlto.bc'
 
 ; Ensure we ignore empty index file under -ignore-empty-index-file, and run
 ; non-ThinLTO compilation which would not import f2
-; RUN: touch %t4.thinlto.bc
-; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t4.o -x ir %t1.o -c -fthinlto-index=%t4.thinlto.bc -mllvm -ignore-empty-index-file
+; RUN: touch %t.empty.thinlto.bc
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t4.o -x ir %t1.o -c -fthinlto-index=%t.empty.thinlto.bc -mllvm -ignore-empty-index-file
 ; RUN: llvm-nm %t4.o | FileCheck --check-prefix=CHECK-OBJ-IGNORE-EMPTY %s
 ; CHECK-OBJ-IGNORE-EMPTY: T f1
 ; CHECK-OBJ-IGNORE-EMPTY: U f2
 
+; Ensure we load the correct module from a multi-module bitcode file
+; 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
+
 ; Ensure f2 was imported
 ; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c -fthinlto-index=%t.thinlto.bc
 ; RUN: llvm-nm %t3.o | FileCheck --check-prefix=CHECK-OBJ %s
 ; CHECK-OBJ: T f1
 ; CHECK-OBJ-NOT: U f2
 
 ; Ensure we get expected error for input files without summaries
 ; RUN: opt -o %t2.o %s