[OpenMP] Add a flag for embedding a file into the module

[jhuber6]

Summary

This patch adds support for a flag -fembed-offload-binary to embed a
file as an ELF section in the output by placing it in a global variable.
This can be used to bundle offloading files with the host binary so it
can be accessed by the linker. The section is named using the
-fembed-offload-section option.

Depends on D116541

[jhuber6]

Clang format

[JonChesterfield]

I don't think this works for multiple files. The test only tries one and misses all the parsing handling.

clang/include/clang/Basic/CodeGenOptions.h

279

This is unclear. List in what sense? It's a std::string, which suggests it's a freeform argument that gets parsed somewhere else.

The relation between the two strings is also unclear. Are the lists meant to be the same length, implying a one to one mapping? If there is a strong relation between the two we can probably remove a bunch of error handling by taking one argument instead of two.

Perhaps the variable should be something like a vector<pair<filename, section>>?

clang/lib/CodeGen/BackendUtil.cpp

1758

Definitely don't want to assert on invalid commandline argument

1775

This looks lossy - if two files use the same section name, they'll end up appended in an order that is probably an implementation quirk of llvm-link, and I think we've thrown away the filename info so can't get back to where we were.

Would .llvm.offloading.filename be a reasonable name for each section, with either error on duplicates or warning + discard?

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

4981

I think I've written some handling very like this in an LDS pass that I meant to factor out for reuse but haven't got around to - we're removing a value from compiler.used?

4982

This removes all variables with that prefix

4993

missing end of comment

5002

Is this necessary? 1 seems a likely default for a uint8_t array

5006

And this pushes one global back into that array

So this looks like it'll mark the last embedded file as .used and none of the others

[JonChesterfield]

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

4890

This seems to be where the code was copied from. Looks broken here too, unless there's some constraint which means exactly one things is embedded at a time - which might be the case given the .module suffix

[JonChesterfield]

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

4933

here's an assert that this function only handled one at a time

4967

This is probably better written in terms of a call to a function like removeFromUsedList, which exists in AMDGPULowerModuleLDSPass at present because I forgot to move it and failed to find an existing implementation. Perhaps something that should be next to llvm::collectUsedGlobalVariables in Module.cpp?

[jhuber6]

clang/include/clang/Basic/CodeGenOptions.h

279

It's a list of comma separated values, but you're right. This should be parsed out when we handle the flags.

clang/lib/CodeGen/BackendUtil.cpp

1758

Will remove.

1775

We only care about the sections per-file right. When I extract these in the linker-wrapper I simply look at each file's sections, and put them into a list of device inputs, we don't need them to be unique as long as there aren't multiple in the same file.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

4981

This handling of the compiler.used variable was copied from the implementation of -fembed-bitcode above. I wasn't sure about their methodology but I figured it worked for them. I can tidy it up to be more straightforward.

4993

Will fix.

5002

Will remove.

[jhuber6]

Updating approach, use a vector of string pairs now. Multiple files are simply
passed multiple times. Will add filename to the offloading section name laterf,
as similar sections could be merged when performing a relocatable link.

[jhuber6]

llvm/lib/Bitcode/Writer/CMakeLists.txt

14 ↗

(On Diff #403404)

I'm not sure if it's worth linking TransformUtils just for appendToCompilerUsed I can copy the implementation locally if not.

[jhuber6]

clang format.

[jhuber6]

Forgot to rename file.

[JonChesterfield]

This might be OK. If multiple translation units still get fed to llvm-link it'll be broken, which might be what we get on amdgpu at present.

Not totally clear to me whether this should be compiler.used or .used, as it's used by something after clang but before the linker. However I recently learned that compiler.used and used are the same thing on elf anyway, so it doesn't matter hugely. Going with compiler.used for now and then backing it off to .used later if something starts deleting them seems fine.

clang/lib/CodeGen/BackendUtil.cpp

1775

I think we'll have problems if multiple files are embedded with the same section string, as they'll get concatenated in the output. llvm-link or ld -r on the host bitcode files will hit that.

It would be worth testing this with two input files, for the same offloading architecture, on amdgpu since I think it will feed the host bitcode to llvm-link which will implicitly concatenate the two embedded files.

clang/test/Frontend/embed-object.ll

3

I think we need a test case with more than one embedded file, given there's the careful splitting around commas in the implementation

llvm/lib/Bitcode/Writer/CMakeLists.txt

14 ↗

(On Diff #403404)

Definitely better to link in an LLVM library than to copy&paste functions out of it

[jhuber6]

clang/lib/CodeGen/BackendUtil.cpp

1775

This scheme works on AMDGPU because we don't use llvm-link as a part of the driver anymore, the new scheme unifies the behavior between NVPTX and AMDGPU until we hit the linker wrapper. But you're right that if the user creates host bitcode and runs llvm-link on that, or performs a relocatable link, we'll get conflicts. I can add a unique string at the end of the section name to avoid this in a later patch.

clang/test/Frontend/embed-object.ll

3

I only split to create a pair now, I can make a test where we pass this flag multiple times.

[jhuber6]

Adding test for multiple files (added it to wrong commit).

[jhuber6]

Changing the name to be the section name. This ensures that if the sections get merged we will get a linker error without failing silently.

[JonChesterfield]

clang/lib/CodeGen/BackendUtil.cpp

1754

Could we have a type here instead of auto? I'm trying to guess what a StringRef split might return and there seems to be a few choices. If it's a container of some sort we could error on size() != 2

[JonChesterfield]

clang/lib/CodeGen/BackendUtil.cpp

1765

OK, so on failure to parse (missing comma?), this will be an empty string, and the section created will be .llvm.offloading.. Given that sections with the same name are implicitly concatenated that's a sharp edge, can we fatal error on the second field being empty? It indicate a badly formed commandline argument of some sort

[JonChesterfield]

clang/lib/CodeGen/BackendUtil.cpp

1754

Nvm, it's a pair.

We could add

if (OffloadObject.count(',') !=1 )  {
    Diags.Report(DiagID) << OffloadObject;
   return;
}

[jhuber6]

Add error handling routine to ensure that the embedding string is always a pair separated by a single ','.

[JonChesterfield]

Thanks for sticking with me on this! LG

[cmtice]

This change introduces a circular dependency: BitcodeWriters now depends on TransformUtils, but TransformUtils also depends on BitcodeWriters. This appears to be a layering violation.

[jhuber6]

In D116542#3285857, @cmtice wrote:

This change introduces a circular dependency: BitcodeWriters now depends on TransformUtils, but TransformUtils also depends on BitcodeWriters. This appears to be a layering violation.

Might explain why it wasn't included before, should I just copy the function I here and remove the dependency?

[MaskRay]

@jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9.. OK, I was late.

See D118666 for the proper fix.

I'd be better to revert this relevant changes if that doesn't make you feel back.
I can prepare the revert.

[jhuber6]

In D116542#3285983, @MaskRay wrote:

@jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9.. OK, I was late.

See D118666 for the proper fix.

I'd be better to revert this relevant changes if that doesn't make you feel back.
I can prepare the revert.

That's fine, I put it here originally because it was grouped with another similar function. But it's likely that one should be moved as well.

[MaskRay]

In D116542#3285985, @jhuber6 wrote:

In D116542#3285983, @MaskRay wrote:

@jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9.. OK, I was late.

See D118666 for the proper fix.

I'd be better to revert this relevant changes if that doesn't make you feel back.
I can prepare the revert.

That's fine, I put it here originally because it was grouped with another similar function. But it's likely that one should be moved as well.

Thanks:) The issue should be resolved by 85dfe19b36ba6e9657612e072c9183ce168fdbbc. No need to revert.

clang/include/clang/Basic/CodeGenOptions.h

public:
 
   /// Prefix to use for -save-temps output.
   std::string SaveTempsFilePrefix;
 
   /// Name of file passed with -fcuda-include-gpubinary option to forward to
   /// CUDA runtime back-end for incorporating them into host-side object file.
   std::string CudaGpuBinaryFileName;
 
+  /// List of file passed with -fembed-offload-binary option to embed
+  /// device-side offloading binaries in the host object file.
+  std::string OffloadBinaryString;
+
+  /// List of section names pass with -fembed-offload-binary to use when
+  /// embedding files passed with -fembed-offload-binary.
+  std::string OffloadSectionString;
+
   /// The name of the file to which the backend should save YAML optimization
   /// records.
   std::string OptRecordFile;
 
   /// The regex that filters the passes that should be saved to the optimization
   /// records.
   std::string OptRecordPasses;
 

clang/include/clang/CodeGen/BackendUtil.h

namespace clang {
   void EmitBackendOutput(DiagnosticsEngine &Diags, const HeaderSearchOptions &,
                          const CodeGenOptions &CGOpts,
                          const TargetOptions &TOpts, const LangOptions &LOpts,
                          StringRef TDesc, llvm::Module *M, BackendAction Action,
                          std::unique_ptr<raw_pwrite_stream> OS);
 
   void EmbedBitcode(llvm::Module *M, const CodeGenOptions &CGOpts,
                     llvm::MemoryBufferRef Buf);
+
+  void EmbedBinary(llvm::Module *M, const CodeGenOptions &CGOpts,
+                   DiagnosticsEngine &Diags);
 }
 
 #endif

clang/include/clang/Driver/Options.td

HelpText<"Specify comma-separated list of offloading target triples"
            " (HIP only)">;
 
 // C++ Coroutines TS
 defm coroutines_ts : BoolFOption<"coroutines-ts",
   LangOpts<"Coroutines">, Default<cpp20.KeyPath>,
   PosFlag<SetTrue, [CC1Option], "Enable support for the C++ Coroutines TS">,
   NegFlag<SetFalse>>;
 
+def fembed_offload_binary_EQ : Joined<["-"], "fembed-offload-binary=">,
+  Group<f_Group>, Flags<[NoXarchOption, CC1Option]>,
+  HelpText<"Embed Offloading device-side binary into host object file.">,
+  MarshallingInfoString<CodeGenOpts<"OffloadBinaryString">>;
+def fembed_offload_section_EQ : Joined<["-"], "fembed-offload-section=">,
+  Group<f_Group>, Flags<[NoXarchOption, CC1Option]>,
+  HelpText<"Section name to use for the embedded device binary.">,
+  MarshallingInfoString<CodeGenOpts<"OffloadSectionString">>;
 def fembed_bitcode_EQ : Joined<["-"], "fembed-bitcode=">,
     Group<f_Group>, Flags<[NoXarchOption, CC1Option, CC1AsOption]>, MetaVarName<"<option>">,
     HelpText<"Embed LLVM bitcode (option: off, all, bitcode, marker)">,
     Values<"off,all,bitcode,marker">, NormalizedValuesScope<"CodeGenOptions">,
     NormalizedValues<["Embed_Off", "Embed_All", "Embed_Bitcode", "Embed_Marker"]>,
     MarshallingInfoEnum<CodeGenOpts<"EmbedBitcode">, "Embed_Off">;
 def fembed_bitcode : Flag<["-"], "fembed-bitcode">, Group<f_Group>,
   Alias<fembed_bitcode_EQ>, AliasArgs<["all"]>,

clang/lib/CodeGen/BackendUtil.cpp

 }
 
 // With -fembed-bitcode, save a copy of the llvm IR as data in the
 // __LLVM,__bitcode section.
 void clang::EmbedBitcode(llvm::Module *M, const CodeGenOptions &CGOpts,
                          llvm::MemoryBufferRef Buf) {
   if (CGOpts.getEmbedBitcode() == CodeGenOptions::Embed_Off)
     return;
+
   llvm::EmbedBitcodeInModule(
       *M, Buf, CGOpts.getEmbedBitcode() != CodeGenOptions::Embed_Marker,
       CGOpts.getEmbedBitcode() != CodeGenOptions::Embed_Bitcode,
       CGOpts.CmdArgs);
 }
+
+void clang::EmbedBinary(llvm::Module *M, const CodeGenOptions &CGOpts,
+    DiagnosticsEngine &Diags) {
+  if (CGOpts.OffloadBinaryString.empty())
+    return;
+
+  SmallVector<StringRef, 4> BinaryFilenames;
+  SmallVector<StringRef, 4> BinarySections;
+  StringRef(CGOpts.OffloadBinaryString).split(BinaryFilenames, ",");
+  StringRef(CGOpts.OffloadSectionString).split(BinarySections, ",");
+
+  assert(BinaryFilenames.size() == BinarySections.size() &&
+         "Different number of filenames and section names in embedding");
+
+  auto BinarySection = BinarySections.begin();
+  for (StringRef BinaryFilename : BinaryFilenames) {
+    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> BinaryOrErr =
+      llvm::MemoryBuffer::getFileOrSTDIN(BinaryFilename);
+    if (std::error_code EC = BinaryOrErr.getError()) {
+      auto DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
+          "could not open '%0' for embedding");
+      Diags.Report(DiagID) << BinaryFilename;
+      return;
+    }
+
+    SmallString<128> SectionName(".llvm.offloading");
+    if (!BinarySection->empty()) {
+      SectionName += ".";
+      SectionName += *BinarySection;
+    }
+    llvm::EmbedObjectInModule(*M, **BinaryOrErr, SectionName);
+    ++BinarySection;
+  }
+}

clang/lib/CodeGen/CodeGenAction.cpp

void CodeGenAction::ExecuteAction() {
 
   const TargetOptions &TargetOpts = CI.getTargetOpts();
   if (TheModule->getTargetTriple() != TargetOpts.Triple) {
     Diagnostics.Report(SourceLocation(), diag::warn_fe_override_module)
         << TargetOpts.Triple;
     TheModule->setTargetTriple(TargetOpts.Triple);
   }
 
+  EmbedBinary(TheModule.get(), CodeGenOpts, Diagnostics);
   EmbedBitcode(TheModule.get(), CodeGenOpts, *MainFile);
 
   LLVMContext &Ctx = TheModule->getContext();
 
   // Restore any diagnostic handler previously set before returning from this
   // function.
   struct RAII {
     LLVMContext &Ctx;

clang/test/Frontend/embed-object.ll

+; RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm \
+; RUN:    -fembed-offload-binary=%S/Inputs/empty.h -fembed-offload-section=section -x ir %s -o - \
+; RUN:    | FileCheck %s -check-prefix=CHECK
+
+; CHECK: @llvm.embedded.object = private constant [0 x i8] zeroinitializer, section ".llvm.offloading.section", align 1
+; CHECK: @llvm.compiler.used = appending global [2 x i8*] [i8* @x, i8* getelementptr inbounds ([0 x i8], [0 x i8]* @llvm.embedded.object, i32 0, i32 0)], section "llvm.metadata"
+
+@x = private constant i8 1
[e-mail] = appending global [1 x i8*] [i8* @x], section "llvm.metadata"
+
+define i32 @foo() {
+  ret i32 0
+}

llvm/include/llvm/Bitcode/BitcodeWriter.h

class raw_ostream;
   /// is not bitcode (i.e. if it's invalid data or even textual LLVM assembly).
   /// If EmbedCmdline is set, the command line is also exported in
   /// the corresponding section (__LLVM,_cmdline / .llvmcmd) - even if CmdArgs
   /// were empty.
   void EmbedBitcodeInModule(Module &M, MemoryBufferRef Buf, bool EmbedBitcode,
                             bool EmbedCmdline,
                             const std::vector<uint8_t> &CmdArgs);
 
+  /// Embeds the memory buffer \p Buf into the module \p M as a global using the
+  /// section name \p SectionName.
+  void EmbedObjectInModule(Module &M, MemoryBufferRef Buf,
+                           StringRef SectionName);
+
 } // end namespace llvm
 
 #endif // LLVM_BITCODE_BITCODEWRITER_H

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

 void llvm::EmbedBitcodeInModule(llvm::Module &M, llvm::MemoryBufferRef Buf,
                                 bool EmbedBitcode, bool EmbedCmdline,
                                 const std::vector<uint8_t> &CmdArgs) {
   // Save llvm.compiler.used and remove it.
   SmallVector<Constant *, 2> UsedArray;
   SmallVector<GlobalValue *, 4> UsedGlobals;
   Type *UsedElementType = Type::getInt8Ty(M.getContext())->getPointerTo(0);
   GlobalVariable *Used = collectUsedGlobalVariables(M, UsedGlobals, true);
   for (auto *GV : UsedGlobals) {
     if (GV->getName() != "llvm.embedded.module" &&
         GV->getName() != "llvm.cmdline")
       UsedArray.push_back(
           ConstantExpr::getPointerBitCastOrAddrSpaceCast(GV, UsedElementType));
   }
   if (Used)
     Used->eraseFromParent();
 
void llvm::EmbedBitcodeInModule(llvm::Module &M, llvm::MemoryBufferRef Buf,
   // Set alignment to 1 to prevent padding between two contributions from input
   // sections after linking.
   GV->setAlignment(Align(1));
   UsedArray.push_back(
       ConstantExpr::getPointerBitCastOrAddrSpaceCast(GV, UsedElementType));
   if (llvm::GlobalVariable *Old =
           M.getGlobalVariable("llvm.embedded.module", true)) {
     assert(Old->hasOneUse() &&
            "llvm.embedded.module can only be used once in llvm.compiler.used");
     GV->takeName(Old);
     Old->eraseFromParent();
   } else {
     GV->setName("llvm.embedded.module");
   }
 
   // Skip if only bitcode needs to be embedded.
   if (EmbedCmdline) {
if (EmbedCmdline) {
     } else {
       GV->setName("llvm.cmdline");
     }
   }
 
   if (UsedArray.empty())
     return;
 
   // Recreate llvm.compiler.used.
   ArrayType *ATy = ArrayType::get(UsedElementType, UsedArray.size());
   auto *NewUsed = new GlobalVariable(
       M, ATy, false, llvm::GlobalValue::AppendingLinkage,
       llvm::ConstantArray::get(ATy, UsedArray), "llvm.compiler.used");
   NewUsed->setSection("llvm.metadata");
 }
+
+void llvm::EmbedObjectInModule(llvm::Module &M, llvm::MemoryBufferRef Buf,
+                               StringRef SectionName) {
+  // Save llvm.compiler.used and remove it.
+  SmallVector<Constant *, 2> UsedArray;
+  SmallVector<GlobalValue *, 4> UsedGlobals;
+  Type *UsedElementType = Type::getInt8Ty(M.getContext())->getPointerTo(0);
+  GlobalVariable *Used = collectUsedGlobalVariables(M, UsedGlobals, true);
+  for (auto *GV : UsedGlobals) {
+    if (!GV->getName().startswith("llvm.embedded.object"))
+      UsedArray.push_back(
+          ConstantExpr::getPointerBitCastOrAddrSpaceCast(GV, UsedElementType));
+  }
+  if (Used)
+    Used->eraseFromParent();
+
+  ArrayRef<uint8_t> ModuleData = ArrayRef<uint8_t>(
+      (const uint8_t *)Buf.getBufferStart(), Buf.getBufferSize());
+
+  // Embed the data in the 
+  llvm::Constant *ModuleConstant =
+      llvm::ConstantDataArray::get(M.getContext(), ModuleData);
+  llvm::GlobalVariable *GV = new llvm::GlobalVariable(
+      M, ModuleConstant->getType(), true, llvm::GlobalValue::PrivateLinkage,
+      ModuleConstant, "llvm.embedded.object");
+  GV->setSection(SectionName);
+  // Set alignment to 1 to prevent padding between two contributions from input
+  // sections after linking.
+  GV->setAlignment(Align(1));
+  UsedArray.push_back(
+      ConstantExpr::getPointerBitCastOrAddrSpaceCast(GV, UsedElementType));
+
+  // Recreate llvm.compiler.used.
+  ArrayType *ATy = ArrayType::get(UsedElementType, UsedArray.size());
+  auto *NewUsed = new GlobalVariable(
+      M, ATy, false, llvm::GlobalValue::AppendingLinkage,
+      llvm::ConstantArray::get(ATy, UsedArray), "llvm.compiler.used");
+  NewUsed->setSection("llvm.metadata");
+}