[Clang] Allow bitcode linking when the input is LLVM-IR

[jhuber6]

Summary

Clang provides the -mlink-bitcode-file and -mlink-builtin-bitcode
options to insert LLVM-IR into the current TU. These are usefuly
primarily for including LLVM-IR files that require special handling to
be correct and cannot be linked normally, such as GPU vendor libraries
like libdevice.10.bc. Currently these options can only be used if the
source input goes through the AST consumer path. This patch makes the
changes necessary to also support this when the input is LLVM-IR. This
will allow the following operation:

clang in.bc -Xclang -mlink-builtin-bitcode -Xclang libdevice.10.bc

[JonChesterfield]

That's cleaner than I expected, thanks. Might be reasonable to factor out the method as an initial NFC then insert the call to it along with the new test case as the functional change.

[tra]

clang in.bc -Xclang -mlink-builtin-bitcode -Xclang libdevice.10.bc

If that's something we intend to expose to the user, should we consider promoting it to a top-level driver option?

[jhuber6]

In D152391#4404106, @tra wrote:

clang in.bc -Xclang -mlink-builtin-bitcode -Xclang libdevice.10.bc

If that's something we intend to expose to the user, should we consider promoting it to a top-level driver option?

I'm not sure, this probably wouldn't be relevant for non-compiler developers so I'm fine keeping it as an -Xclang workaround.

[jhuber6]

Just realized this is probably going to be a bit more painful. the attribute propagation pass requires a CodeGenModule which isn't built without an ASTContext so it's not available here. Nothing those functions do explicitly requires the full CGM, they only use the options and the target info. So it's possible to make these functions more generic using only the CompilerInstance instead, but it'll be a lot nosier.

[jhuber6]

Scratch that, ASTContext::getFunctionFeatureMap is used. I wonder if we could forgo that.

Actually, that's only used if there's a GlobalDecl which we don't have when we call this. So that could be ignored here.

[jhuber6]

Updating, in order to do this starting with bitcode I had to expose a helper
that performs this operation using the options directly rather than through the
CodeGenModule. This should keep the existing interfaces intact by shuttling
them through a new internal function while also letting me write this standalone
function that can be used without a CodeGenModule.

[jhuber6]

Add a better test to show that attributes are replaced and default attributes are added.

[jhuber6]

ping

[yaxunl]

clang/lib/CodeGen/CGCall.cpp

2052–2106 ↗

(On Diff #529557)

can we reorder these functions to minimize the diffs? Also some comments about the difference among these functions may help.

clang/lib/CodeGen/CodeGenAction.cpp

265

Removing the default argument may make the code clearer

[jhuber6]

Addressing comments.

[jhuber6]

clang/lib/CodeGen/CGCall.cpp

2052–2106 ↗

(On Diff #529557)

I added some comments. These can't be reordered because they're new functions, I simply added a layer of indirection to go through a common helper so we can call the main bulk of the code without needing the full CGM.

[yaxunl]

clang/lib/CodeGen/CGCall.cpp

2105–2106 ↗

(On Diff #530919)

should this be removed? seems redundant

[jhuber6]

clang/lib/CodeGen/CGCall.cpp

2105–2106 ↗

(On Diff #530919)

I wasn't entirely sure, there's a lot of work done in that function but pretty much all of it goes away since we just use GlobalDecl(). So for the version that doesn't have access to CGM I copies out the only two relevant lines and put them in line 2063. We could probably make this use the same code but I wasn't sure if there was something else we might need it for.

[yaxunl]

clang/lib/CodeGen/CGCall.cpp

2105–2106 ↗

(On Diff #530919)

I tend to think they are equivalent. If GetCPUAndFeaturesAttributes did something else, it may be overwritten by the counterparts in mergeDefaultFunctionDefinitionAttributes.

[jhuber6]

Removing

[jhuber6]

Format

[JonChesterfield]

clang/lib/CodeGen/CGCall.cpp

2001 ↗

(On Diff #530946)

I'm used to this sort of copy-some-args-and-not-others showing up in bug reports. Could this patch be re-ordered to make it apparent what functional changes are happening relative to the current head?

[jhuber6]

clang/lib/CodeGen/CGCall.cpp

2001 ↗

(On Diff #530946)

It's the same that will get added in in the GetCPUAndFeaturesAttributes function if you pass in a null declaration.

[jhuber6]

ping
@ronlieb

[yaxunl]

LGTM. Thanks.

[MaskRay]

LGTM. CC1 -mlink-bitcode-file is an interesting option from 2011: f1d76db466b2a50781c0754b86ac994dd07b5041
I wonder what the original use case is...

clang/include/clang/CodeGen/CodeGenAction.h

private:
 
   /// Bitcode modules to link in to our module.
   SmallVector<LinkModule, 4> LinkModules;
   llvm::LLVMContext *VMContext;
   bool OwnsVMContext;
 
   std::unique_ptr<llvm::Module> loadModule(llvm::MemoryBufferRef MBRef);
 
+  /// Load bitcode modules to link into our module from the options.
+  bool loadLinkModules(CompilerInstance &CI);
+
 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/CodeGenAction.cpp

public:
 
     void HandleInterestingDecl(DeclGroupRef D) override {
       // Ignore interesting decls from the AST reader after IRGen is finished.
       if (!IRGenFinished)
         HandleTopLevelDecl(D);
     }
 
     // Links each entry in LinkModules into our module.  Returns true on error.
-    bool LinkInModules() {
+    bool LinkInModules(llvm::Module *M = nullptr) {
       for (auto &LM : LinkModules) {
         assert(LM.Module && "LinkModule does not actually have a module");
         if (LM.PropagateAttrs)
           for (Function &F : *LM.Module) {
             // Skip intrinsics. Keep consistent with how intrinsics are created
             // in LLVM IR.
             if (F.isIntrinsic())
               continue;
             Gen->CGM().mergeDefaultFunctionDefinitionAttributes(F,
                                                                 LM.Internalize);
           }
 
         CurLinkModule = LM.Module.get();
 
         bool Err;
         if (LM.Internalize) {
           Err = Linker::linkModules(
-              *getModule(), std::move(LM.Module), LM.LinkFlags,
+              M ? *M : *getModule(), std::move(LM.Module), LM.LinkFlags,
               [](llvm::Module &M, const llvm::StringSet<> &GVS) {
                 internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
                   return !GV.hasName() || (GVS.count(GV.getName()) == 0);
                 });
               });
         } else {
-          Err = Linker::linkModules(*getModule(), std::move(LM.Module),
+          Err = Linker::linkModules(M ? *M : *getModule(), std::move(LM.Module),
                                     LM.LinkFlags);
         }
 
         if (Err)
           return true;
       }
       LinkModules.clear();
       return false; // success
: Act(_Act), VMContext(_VMContext ? _VMContext : new LLVMContext),
       OwnsVMContext(!_VMContext) {}
 
 CodeGenAction::~CodeGenAction() {
   TheModule.reset();
   if (OwnsVMContext)
     delete VMContext;
 }
 
+bool CodeGenAction::loadLinkModules(CompilerInstance &CI) {
+  if (!LinkModules.empty())
+    return false;
+
+  for (const CodeGenOptions::BitcodeFileToLink &F :
+       CI.getCodeGenOpts().LinkBitcodeFiles) {
+    auto BCBuf = CI.getFileManager().getBufferForFile(F.Filename);
+    if (!BCBuf) {
+      CI.getDiagnostics().Report(diag::err_cannot_open_file)
+          << F.Filename << BCBuf.getError().message();
+      LinkModules.clear();
+      return true;
+    }
+
+    Expected<std::unique_ptr<llvm::Module>> ModuleOrErr =
+        getOwningLazyBitcodeModule(std::move(*BCBuf), *VMContext);
+    if (!ModuleOrErr) {
+      handleAllErrors(ModuleOrErr.takeError(), [&](ErrorInfoBase &EIB) {
+        CI.getDiagnostics().Report(diag::err_cannot_open_file)
+            << F.Filename << EIB.message();
+      });
+      LinkModules.clear();
+      return true;
+    }
+    LinkModules.push_back({std::move(ModuleOrErr.get()), F.PropagateAttrs,
+                           F.Internalize, F.LinkFlags});
+  }
+  return false;
+}
+
 bool CodeGenAction::hasIRSupport() const { return true; }
 
 void CodeGenAction::EndSourceFileAction() {
   // If the consumer creation failed, do nothing.
   if (!getCompilerInstance().hasASTConsumer())
     return;
 
   // Steal the module from the consumer.
if (!OS)
     OS = GetOutputStream(CI, InFile, BA);
 
   if (BA != Backend_EmitNothing && !OS)
     return nullptr;
 
   VMContext->setOpaquePointers(CI.getCodeGenOpts().OpaquePointers);
 
   // Load bitcode modules to link with, if we need to.
-  if (LinkModules.empty())
-    for (const CodeGenOptions::BitcodeFileToLink &F :
-         CI.getCodeGenOpts().LinkBitcodeFiles) {
-      auto BCBuf = CI.getFileManager().getBufferForFile(F.Filename);
-      if (!BCBuf) {
-        CI.getDiagnostics().Report(diag::err_cannot_open_file)
-            << F.Filename << BCBuf.getError().message();
-        LinkModules.clear();
-        return nullptr;
-      }
-
-      Expected<std::unique_ptr<llvm::Module>> ModuleOrErr =
-          getOwningLazyBitcodeModule(std::move(*BCBuf), *VMContext);
-      if (!ModuleOrErr) {
-        handleAllErrors(ModuleOrErr.takeError(), [&](ErrorInfoBase &EIB) {
-          CI.getDiagnostics().Report(diag::err_cannot_open_file)
-              << F.Filename << EIB.message();
-        });
-        LinkModules.clear();
-        return nullptr;
-      }
-      LinkModules.push_back({std::move(ModuleOrErr.get()), F.PropagateAttrs,
-                             F.Internalize, F.LinkFlags});
-    }
+  if (loadLinkModules(CI))
 
   CoverageSourceInfo *CoverageInfo = nullptr;
   // Add the preprocessor callback only when the coverage mapping is generated.
   if (CI.getCodeGenOpts().CoverageMapping)
     CoverageInfo = CodeGen::CoverageMappingModuleGen::setUpCoverageCallbacks(
         CI.getPreprocessor());
 
   std::unique_ptr<BackendConsumer> Result(new BackendConsumer(
if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty()) {
     }
     Expected<std::unique_ptr<llvm::Module>> MOrErr =
         Bm->parseModule(*VMContext);
     if (!MOrErr)
       return DiagErrors(MOrErr.takeError());
     return std::move(*MOrErr);
   }
 
+  // Load bitcode modules to link with, if we need to.
+  if (loadLinkModules(CI))
+    return nullptr;
+
   llvm::SMDiagnostic Err;
   if (std::unique_ptr<llvm::Module> M = parseIR(MBRef, Err, *VMContext))
     return M;
 
   // Translate from the diagnostic info to the SourceManager location if
   // available.
   // TODO: Unify this with ConvertBackendLocation()
   SourceLocation Loc;
void CodeGenAction::ExecuteAction() {
 
   // Set clang diagnostic handler. To do this we need to create a fake
   // BackendConsumer.
   BackendConsumer Result(BA, CI.getDiagnostics(), &CI.getVirtualFileSystem(),
                          CI.getHeaderSearchOpts(), CI.getPreprocessorOpts(),
                          CI.getCodeGenOpts(), CI.getTargetOpts(),
                          CI.getLangOpts(), TheModule.get(),
                          std::move(LinkModules), *VMContext, nullptr);
+  if (Result.LinkInModules(&*TheModule))
+    return;
   // PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
   // true here because the valued names are needed for reading textual IR.
   Ctx.setDiscardValueNames(false);
   Ctx.setDiagnosticHandler(
       std::make_unique<ClangDiagnosticHandler>(CodeGenOpts, &Result));
 
   Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
       setupLLVMOptimizationRemarks(

clang/test/CodeGen/link-bitcode-file.c

 // RUN: %clang_cc1 -triple i386-pc-linux-gnu -DBITCODE -emit-llvm-bc -o %t.bc %s
 // RUN: %clang_cc1 -triple i386-pc-linux-gnu -DBITCODE2 -emit-llvm-bc -o %t-2.bc %s
 // RUN: %clang_cc1 -triple i386-pc-linux-gnu -mlink-bitcode-file %t.bc \
 // RUN:     -O3 -emit-llvm -o - %s | FileCheck -check-prefix=CHECK-NO-BC %s
 // RUN: %clang_cc1 -triple i386-pc-linux-gnu -O3 -emit-llvm -o - \
 // RUN:     -mlink-bitcode-file %t.bc -mlink-bitcode-file %t-2.bc %s \
 // RUN:     | FileCheck -check-prefix=CHECK-NO-BC -check-prefix=CHECK-NO-BC2 %s
 // RUN: not %clang_cc1 -triple i386-pc-linux-gnu -DBITCODE -O3 -emit-llvm -o - \
 // RUN:     -mlink-bitcode-file %t.bc %s 2>&1 | FileCheck -check-prefix=CHECK-BC %s
 // Make sure we deal with failure to load the file.
 // RUN: not %clang_cc1 -triple i386-pc-linux-gnu -mlink-bitcode-file no-such-file.bc \
 // RUN:    -emit-llvm -o - %s 2>&1 | FileCheck -check-prefix=CHECK-NO-FILE %s
 
+// Make sure we can perform the same options if the input is LLVM-IR
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -emit-llvm-bc -o %t-in.bc %s
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -mlink-bitcode-file %t.bc \
+// RUN:     -O3 -emit-llvm -o - %t-in.bc | FileCheck -check-prefix=CHECK-NO-BC %s
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -O3 -emit-llvm -o - \
+// RUN:     -mlink-bitcode-file %t.bc -mlink-bitcode-file %t-2.bc %t-in.bc \
+// RUN:     | FileCheck -check-prefix=CHECK-NO-BC -check-prefix=CHECK-NO-BC2 %s
+
 int f(void);
 
 #ifdef BITCODE
 
 extern int f2(void);
 // CHECK-BC: fatal error: cannot link module {{.*}}'f': symbol multiply defined
 int f(void) {
   f2();