[Object] Add binary format for bundling offloading metadata

[jhuber6]

Summary

We need to embed certain metadata along with a binary image when we wish
to perform a device-linking job on it. Currently this metadata was
embedded in the section name of the data itself. This worked, but made
adding new metadata very difficult and didn't work if the user did any
sort of section linking.

This patch introduces a custom binary format for bundling offloading
metadata with a device object file. This binary format is fundamentally
a simple string map table with some additional data and an embedded
image. I decided to use a custom format rather than using an existing
format (ELF, JSON, etc) because of the specialty use-case of this. We
need a simple binary format that can be concatenated without requiring
other external dependencies.

This extension will make it easier to extend the linker wrapper's
capabilties with whatever data is necessary. Eventually this will allow
us to remove all the external arguments passed to the linker wrapper and
embed it directly in the host's linker so device linking behaves exactly
like host linking.

[jhuber6]

clang/include/clang/Basic/Offloading.h

2

Not sure if this should live in Clang or LLVM.

[jhuber6]

Update alignment to worse-case padding of 16. I'm not sure if there's a bette solution for preventing the linker from adding padding between these sections if they are combined.

[jhuber6]

Fix test after increasing alignment.

[jhuber6]

Changing to add alignment to the global variable. Should ensure that the section
alignment is correct.

[tra]

I decided to use a custom format rather than using an existing format (ELF, JSON, etc) because of the specialty use-case of this.

Will we ever need to process the files with tools built with a different LLVM version? E.g clang and lld may be built separately from different LLVM trees.
If so, then maintaining compatibility of the binary format will become more painful over time, compared to using json.

Considering that we already have json parsing and serialization in LLVM, I don't see much benefit growing yet another on-disk format. AFAICT, JSON should do the job encoding relevant data just fine and would need less maintenance long term.

clang/include/clang/Basic/Offloading.h

88

On-disk format could use more comments.

88–105

Given that it's an on-disk format I think these should be explicitly packed and carry a comment that it's an on-disk format which needs extra caution during future changes.

91

What does Size cover and what units it uses -- bytes, number of Entries, something else?

96–97

Should we use the matching enum types here?
We're using 16-bit enums, so it would save us on casts when we use those fields and would make it harder to set a wrong value by mistake.

98

Do these flags have defined meaning?

99

What are the offsets relative to?

[jhuber6]

In D122069#3397373, @tra wrote:

I decided to use a custom format rather than using an existing format (ELF, JSON, etc) because of the specialty use-case of this.

Will we ever need to process the files with tools built with a different LLVM version? E.g clang and lld may be built separately from different LLVM trees.
If so, then maintaining compatibility of the binary format will become more painful over time, compared to using json.

As it stands now, this is only used by the linker-wrapper which is all clang. LLD is only called as part of the host link job so it's impossible to mix versions. That being said, eventually I want this functionality to be performed by the linker itself without needing the extra wrapper so it'll be possible in the future. I added the "Version" field specifically to emit a warning or error in the future if we change this. Even if we used JSON or some other form it would be a similar story if these get de-synced because we'd have different entries and keys. We could add to the struct without breaking the ABI since everything uses absolute offsets.

Considering that we already have json parsing and serialization in LLVM, I don't see much benefit growing yet another on-disk format. AFAICT, JSON should do the job encoding relevant data just fine and would need less maintenance long term.

I was definitely thinking about a lot of alternatives to making yet another on-disk binary format. I had a few discussions with @JonChesterfield on the subject. There were two things that put me off of using JSON primarily, correct me if I have any misconceptions.

1. This is fundamentally a very thin metadata wrapper around a binary image. AFAIK if you want to stream binary data to a JSON you need to use a base64 or similar encoding, which makes the files bigger and adds some extra work. It's not a deal-breaker, but it's somewhat of a turn-off.
2. There could be multiple of these contained in a single section, I primarily wanted something with a known size and easy to identify magic bytes to find where these live in the section. I wasn't sure how nicely this would interact with the JSON parser. It could also be done, but I wasn't sure how much utility there was.

clang/include/clang/Basic/Offloading.h

88

Noted, will add more if we settle on this format.

88–105

What do you mean by explicitly packed? And I added the "Version" field in the header so we can warn on old versions.

91

Size of the entire file, so you can do Data[Header->Size] and potentially get the next one in memory.

96–97

Yeah, the casts are annoying but I elected to be explicit with the size constraints in the header itself. Wasn't really sure which was better.

98

They're unused for now, I was planning on making it a bitfield so we could pass things like whether or not this file contains debug information or is optimized, kind of like fatbinary.

99

Absolute from the start of the file, can comment.

[tra]

In D122069#3397560, @jhuber6 wrote:

I was definitely thinking about a lot of alternatives to making yet another on-disk binary format. I had a few discussions with @JonChesterfield on the subject. There were two things that put me off of using JSON primarily, correct me if I have any misconceptions.

1. This is fundamentally a very thin metadata wrapper around a binary image. AFAIK if you want to stream binary data to a JSON you need to use a base64 or similar encoding, which makes the files bigger and adds some extra work.

I didn't realize that content of the file is part of your packagin scheme. I've interpreted embed certain metadata along with a binary image as literally keeping the binaries as binaries and just adding a small blob with additional metadata. It was that data I meant to encode as JSON. Encoding the offload binaries themselves as JSON would indeed be wasteful.

We could keep the header as a binary (never changes on-disk format) and use JSON representation for the array of the entries (0-terminated string or string + length stored in the header) which also has fixed (as in version-agnostic) on-disk format, though of variable length.
Versioning still has to be dealt with, but now it would be independent of the on-disk format. The variable length of JSON is both a plus and a minus. On the positive side is that content is open. Some tool may add whatever is relevant for its own use, comments, provenance info, checksum, etc.
The downside is that it has variable length, so it would have to be written after the image binary. We would also need to deal with potential errors parsing JSON.

I don't have a strong preference either way. I think JSON may have few minor benefits, but the proposed binary format has the advantage of simplicity. We can always switch to json-encoded entries later by bumping the header version.

clang/include/clang/Basic/Offloading.h

88–105

__attribute__((packed)). Otherwise you depend on assumed natural alignment and that is target-dependent, IIRC.

[jhuber6]

In D122069#3397867, @tra wrote:

In D122069#3397560, @jhuber6 wrote:

I was definitely thinking about a lot of alternatives to making yet another on-disk binary format. I had a few discussions with @JonChesterfield on the subject. There were two things that put me off of using JSON primarily, correct me if I have any misconceptions.

1. This is fundamentally a very thin metadata wrapper around a binary image. AFAIK if you want to stream binary data to a JSON you need to use a base64 or similar encoding, which makes the files bigger and adds some extra work.

I didn't realize that content of the file is part of your packagin scheme. I've interpreted embed certain metadata along with a binary image as literally keeping the binaries as binaries and just adding a small blob with additional metadata. It was that data I meant to encode as JSON. Encoding the offload binaries themselves as JSON would indeed be wasteful.

We could keep the header as a binary (never changes on-disk format) and use JSON representation for the array of the entries (0-terminated string or string + length stored in the header) which also has fixed (as in version-agnostic) on-disk format, though of variable length.
Versioning still has to be dealt with, but now it would be independent of the on-disk format. The variable length of JSON is both a plus and a minus. On the positive side is that content is open. Some tool may add whatever is relevant for its own use, comments, provenance info, checksum, etc.
The downside is that it has variable length, so it would have to be written after the image binary. We would also need to deal with potential errors parsing JSON.

I don't have a strong preference either way. I think JSON may have few minor benefits, but the proposed binary format has the advantage of simplicity. We can always switch to json-encoded entries later by bumping the header version.

I like the idea of keeping the header, we could add an additional field to the header for the size of the entry and I feel like we'd be pretty future-proof if we want to change stuff. I think using this binary format for now is sufficient as long as we keep upgrading it to something more complex an open possibility.

I'm also not sure if I should extend this as a binary format inheriting from LLVM's Binary class. It would be a minimal amount of work but I'm not sure if this use-case warrants extending this to broader LLVM.

[jhuber6]

Add more comments and an entry size field to the header.

[jhuber6]

Updating to use path instead of generic cmdline, makes it a lot easier to pass it. Also just adding a reserved field in case I want to add the cmdline back.

[jhuber6]

Splitting this out into a patch for the format. Adding a unit test and changing
strings to now be an arbitrary string map. Hopefully the move to LLVM proper
won't draw ire for creating another binary format in LLVM.

[jhuber6]

Changing test, uniform_int_distribution doesn't support char or uint8_t according to the standard.

[JonChesterfield]

Added some reviewers. I'd much prefer this used an existing binary format, DIY is prone to errors and maintenance hassles down the road. Don't care as much about which format as about it being one with an existing, tested implementation and ideally existing inspection tools.

[jhuber6]

In D122069#3413960, @JonChesterfield wrote:

Added some reviewers. I'd much prefer this used an existing binary format, DIY is prone to errors and maintenance hassles down the road. Don't care as much about which format as about it being one with an existing, tested implementation and ideally existing inspection tools.

I'm not married to the idea, and worst case scenario we can replace it with something else in the future. I'd like to get something like this working so I can finally make the new driver the default, so I'll just outline the problem and some of the potential solutions.

The issue is that we need to store some metadata along with a binary image so we know how to handle it at a later date. Currently we shove this in the name of the ELF section and parse it there, but that's not idea because more metadata will be needed in the future and it prevents us from doing things like relocatable linking or other merging (e.g. we want to store both an sm_70 and sm_35 image in the same file). So we want a binary format that can store some strings, other data. I'll just go over a brief overview of the options:

YAML / JSON

+ Ubiquitous simple text format for encoding object data
+ In-tree implementation
x Requires encoding to store the device image
x Will need binary padding and size calculation to make sure these merge properly in a section

Protocol Buffers

+ Well-tested
+ Implicit appending, no additional code required to handle merged sections.
x Out-of-tree, requires external dependencies to build and maintain in the future. No other use in Clang / LLVM

ELF

+ Ubiquitous tooling. Object extraction and copying for free
+ Simple key-value storage
x Difficult to calculate size, will need to figure out the size of the buffer and write it in later so we can read multiple appended sections.
x Difficult to create. The Elf object writer is completely tied to the MC backend. YAML2ELF would require base64 or similar again

MSGPACK

+ Exists in-tree in some form and well tested
+ Supports key-value storage
x Doesn't know its size, will need to add padding and a size field

Custom format

+ Relatively simple implementation that solves this specific problem
x No existing tooling support, more error prone

I decided to go with the custom format because it was the simplest to get working for a proof of concept to solve the problem I was immediately facing. I think ELF would be the next best if someones could suggest a way to write the data and get the size. MSGPACK seems to be @JonChesterfield's preferred method because it has a lot of use at AMD, it would work as long as we can figure out its size and get alignment. Let me know what suggestions you have because I really want to move forward with this.

[saiislam]

Hey @jhuber6 , as discussed in multi-company meeting, I think that we will need at least an arch field somewhere in this. We would like to create multi-arch binaries so that runtime can load the compatible one on its own.
You may even consider using TargetID Format to store the list of archs.

[jhuber6]

In D122069#3416694, @saiislam wrote:

Hey @jhuber6 , as discussed in multi-company meeting, I think that we will need at least an arch field somewhere in this. We would like to create multi-arch binaries so that runtime can load the compatible one on its own.
You may even consider using TargetID Format to store the list of archs.

The binary format contains a string map along with some integer fields. I have the getArch() function in the binary that just extracts the value get the "arch" key. This makes it easy to add some arbitrary data so I was planning on simply adding a "features" key as well. Then we can extract the associated image features and decide what to do with the image. I haven't thought of a good solution for allowing multiple compatible architectures, maybe a comma separated list of architectures. You can see the proposed usage right now in D122683 but more will be added.

[jhuber6]

Ping, I'd like to finalize the new driver in time for the GPU newsletter and the LLVM Performance Workshop at CGO.

[jhuber6]

ping

[JonChesterfield]

Couple of nits above but basically I'm convinced. The gnarly part of binary formats is string tables and I'm delighted that part of MC was readily reusable. Wrapping the string table in different bytes to align with the elf format may still be a good idea but it's not an obvious correctness hazard.

I like msgpack as a format but the writing machinery in llvm is not very reusable. Likewise elf is a great format but quite interwoven with MC. Protobuf seems to have the nice property of concatenating objects yielding a valid protobuf but the cost of codegen that isn't presently part of the llvm core, and is a slightly hairy dependency to pull in.

Medium term, factoring out parts of the elf handling for use here (and in lld?) is probably reasonable, but the leading magic bytes here are sufficient that we could detect that in backwards-compat fashion if the release gets ahead of us. The format here is essentially a string map which is likely to meet future requirements from other platforms adequately.

Thanks for sticking with this!

llvm/include/llvm/Object/OffloadBinary.h

74 ↗

(On Diff #418255)

these should probably be returning the enum types

97 ↗

(On Diff #418255)

enums here as well? They have uint16_t specified in the type so layout is stable

llvm/lib/Object/OffloadBinary.cpp

20 ↗

(On Diff #418255)

Not sure Expected<> helps hugely here - stuff only goes wrong as 'parse_failed' or failed to allocate, which is kind of the same thing - so we could return a default-initialized (null) unique_ptr on failure without loss of information

41 ↗

(On Diff #418255)

this is good, string table building is by far the most tedious part of formats like this

[jhuber6]

Maxing suggested changes.

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 filenames and section name pairs passed in using the
-  /// -fembed-offload-object option to embed device-side offloading objects into
-  /// the host as a named section. Input passed in as '<filename>,<section>'
+  /// List of filenames and metadata passed in using the -fembed-offload-object
+  /// option to embed device-side offloading objects into the host as a named
+  /// section. Input passed in as 'file=<filename>,<key>=<metadata>, ...'
   std::vector<std::string> OffloadObjects;
 
   /// 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.

clang/include/clang/Basic/Offloading.h

+//===--- Offloading.h - Utilities for handling offloading code  -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_BASIC_OFFLOADING_H
+#define LLVM_CLANG_BASIC_OFFLOADING_H
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include <memory>
+
+using llvm::Expected;
+using llvm::MemoryBuffer;
+using llvm::MemoryBufferRef;
+using llvm::StringRef;
+using llvm::Twine;
+
+namespace clang {
+
+/// The producer of the offloading action associated with the image.
+enum OffloadKind : uint16_t {
+  OFK_None = 0,
+  OFK_OpenMP,
+  OFK_Cuda,
+  OFK_HIP,
+};
+
+/// The type of contents the offloading image contains.
+enum ImageKind : uint16_t {
+  IMG_None = 0,
+  IMG_Object,
+  IMG_Bitcode,
+  IMG_Cubin,
+  IMG_Fatbinary,
+  IMG_PTX,
+};
+
+/// A generic struct for metadata associated with an offloading file. We need to
+/// associate metadata with an offloading file so we can properly identify it
+/// during the linking phase.
+struct OffloadFile {
+  ImageKind TheImageKind;
+  OffloadKind TheOffloadKind;
+  uint32_t Flags;
+
+  std::string TheTriple; // The target triple associated with this image.
+  std::string Arch;      // The target architecture associated with this image.
+  std::string Features;  // Associated target features.
+  std::string CmdLine;   // Associated command line arguments (e.g. cuda-path).
+  std::string Filename;  // The filename containing the image data.
+};
+
+/// A simple binary serialization of an offloading file. We use this format to
+/// embed the offloading image into the host executable so it can be extracted
+/// and used by the linker.
+class OffloadBinary {
+public:
+  static Expected<std::unique_ptr<OffloadBinary>> create(MemoryBufferRef Data);
+
+  static Expected<std::unique_ptr<MemoryBuffer>> write(OffloadFile &File);
+
+  uint16_t getImageKind() const { return TheEntry->ImageKind; }
+  uint16_t getOffloadKind() const { return TheEntry->OffloadKind; }
+  uint32_t getFlags() const { return TheEntry->Flags; }
+  uint64_t getSize() const { return TheHeader->Size; }
+
+  StringRef getTripleStr() const { return &Data[TheEntry->TripleOffset]; }
+  StringRef getArch() const { return &Data[TheEntry->ArchOffset]; }
+  StringRef getFeatures() const { return &Data[TheEntry->FeaturesOffset]; }
+  StringRef getCmdLine() const { return &Data[TheEntry->CmdLineOffset]; }
+  StringRef getImage() const {
+    return StringRef(&Data[TheEntry->ImageOffset], TheEntry->ImageSize);
+  }
+
+  /// Convert the binary file to an offload file and write the image to the
+  /// associated \p Filename.
+  Expected<OffloadFile> createOffloadFile(StringRef Filename);
+
+private:
+  struct Header {
+    uint8_t Magic[4] = {0x10, 0xFF, 0x10, 0xAD};
+    uint32_t Version;
+    uint64_t Size;
+    uint64_t EntryOffset;
+  };
+
+  struct Entry {
+    uint16_t ImageKind;
+    uint16_t OffloadKind;
+    uint32_t Flags;
+    uint64_t TripleOffset;
+    uint64_t ArchOffset;
+    uint64_t FeaturesOffset;
+    uint64_t CmdLineOffset;
+    uint64_t ImageOffset;
+    uint64_t ImageSize;
+  };
+
+  OffloadBinary(MemoryBufferRef Buffer, const Header *TheHeader,
+                const Entry *TheEntry)
+      : Buffer(Buffer), Data(Buffer.getBufferStart()), TheHeader(TheHeader),
+        TheEntry(TheEntry) {}
+
+  OffloadBinary(const OffloadBinary &Other) = delete;
+
+  MemoryBufferRef Buffer;
+
+  /// Pointer to the beginning of the memory buffer.
+  const char *Data;
+  /// Location of the header within the binary.
+  const Header *TheHeader;
+  /// Location of the metadata entries within the binary.
+  const Entry *TheEntry;
+};
+
+ImageKind getImageKind(StringRef Name);
+StringRef getImageKindName(ImageKind Name);
+
+OffloadKind getOffloadKind(StringRef Name);
+StringRef getOffloadKindName(OffloadKind Name);
+
+} // namespace clang
+#endif

clang/lib/Basic/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
+  MC
   )
 
 find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
 find_first_existing_vc_file("${CLANG_SOURCE_DIR}" clang_vc)
 
 # The VC revision include that we want to generate.
 set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 

 
 add_clang_library(clangBasic
   Attributes.cpp
   Builtins.cpp
   CLWarnings.cpp
   CharInfo.cpp
   CodeGenOptions.cpp
   Cuda.cpp
+  Offloading.cpp
   DarwinSDKInfo.cpp
   Diagnostic.cpp
   DiagnosticIDs.cpp
   DiagnosticOptions.cpp
   ExpressionTraits.cpp
   FileEntry.cpp
   FileManager.cpp
   FileSystemStatCache.cpp

clang/lib/Basic/Offloading.cpp

+//===- Offloading.cpp - Utilities for handling offloading code  -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Basic/Offloading.h"
+
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/MC/StringTableBuilder.h"
+#include "llvm/Support/FileOutputBuffer.h"
+
+using namespace llvm;
+
+static constexpr unsigned Alignment = 16;
+
+namespace clang {
+
+Expected<std::unique_ptr<OffloadBinary>>
+OffloadBinary::create(MemoryBufferRef Buffer) {
+  if (Buffer.getBufferSize() < sizeof(Header) + sizeof(Entry))
+    return createStringError(inconvertibleErrorCode(), "Invalid size");
+
+  // Check for 0x10FF1OAD magic bytes.
+  if (!Buffer.getBuffer().startswith("\x10\xff\x10\xad"))
+    return createStringError(inconvertibleErrorCode(),
+                             "Offloading section missing magic bytes");
+
+  const char *Start = Buffer.getBufferStart();
+  const Header *TheHeader = reinterpret_cast<const Header *>(Start);
+  const Entry *TheEntry =
+      reinterpret_cast<const Entry *>(&Start[TheHeader->EntryOffset]);
+
+  return std::unique_ptr<OffloadBinary>(
+      new OffloadBinary(Buffer, TheHeader, TheEntry));
+}
+
+Expected<std::unique_ptr<MemoryBuffer>>
+OffloadBinary::write(OffloadFile &File) {
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> ImageOrErr =
+      llvm::MemoryBuffer::getFileOrSTDIN(File.Filename);
+  if (std::error_code EC = ImageOrErr.getError())
+    return createFileError(File.Filename, EC);
+
+  MemoryBufferRef Image = **ImageOrErr;
+
+  // Create a string table with all the used strings.
+  StringTableBuilder StrTab(StringTableBuilder::ELF);
+  StrTab.add(File.TheTriple);
+  StrTab.add(File.Arch);
+  StrTab.add(File.Features);
+  StrTab.add(File.CmdLine);
+  StrTab.finalize();
+
+  // Create the header and fill in the offsets. The entry will be directly
+  // placed after the header in memory. Align the total size to the worst-case
+  // padding so they can be placed contiguously in a single section.
+  Header TheHeader;
+  TheHeader.Version = 1;
+  TheHeader.Size = alignTo(sizeof(Header) + sizeof(Entry) +
+                               Image.getBufferSize() + StrTab.getSize(),
+                           Alignment);
+  TheHeader.EntryOffset = sizeof(Header);
+
+  // Create the entry using the string table offsets. The string table will be
+  // placed directly after the entry in memory, and the image after that.
+  Entry TheEntry;
+  TheEntry.ImageKind = File.TheImageKind;
+  TheEntry.OffloadKind = File.TheOffloadKind;
+  TheEntry.Flags = File.Flags;
+
+  TheEntry.TripleOffset =
+      sizeof(Header) + sizeof(Entry) + StrTab.getOffset(File.TheTriple);
+  TheEntry.ArchOffset =
+      sizeof(Header) + sizeof(Entry) + StrTab.getOffset(File.Arch);
+  TheEntry.FeaturesOffset =
+      sizeof(Header) + sizeof(Entry) + StrTab.getOffset(File.Features);
+  TheEntry.CmdLineOffset =
+      sizeof(Header) + sizeof(Entry) + StrTab.getOffset(File.CmdLine);
+  TheEntry.ImageOffset = sizeof(Header) + sizeof(Entry) + StrTab.getSize();
+  TheEntry.ImageSize = Image.getBufferSize();
+
+  SmallVector<char, 256> Data;
+  raw_svector_ostream OS(Data);
+  OS << StringRef(reinterpret_cast<char *>(&TheHeader), sizeof(Header));
+  OS << StringRef(reinterpret_cast<char *>(&TheEntry), sizeof(Entry));
+  StrTab.write(OS);
+  OS << Image.getBuffer();
+
+  // Add final padding to required alignment.
+  assert(TheHeader.Size >= OS.tell() && "Too much data written?");
+  OS.write_zeros(TheHeader.Size - OS.tell());
+  assert(TheHeader.Size == OS.tell() && "Size mismatch");
+
+  return MemoryBuffer::getMemBufferCopy(OS.str());
+}
+
+Expected<OffloadFile> OffloadBinary::createOffloadFile(StringRef Filename) {
+  ImageKind TheImageKind = static_cast<ImageKind>(getImageKind());
+  OffloadKind TheOffloadKind = static_cast<OffloadKind>(getOffloadKind());
+
+  // Write the device image data to the provided filename.
+  Expected<std::unique_ptr<FileOutputBuffer>> OutputOrErr =
+      FileOutputBuffer::create(Filename, getImage().size());
+  if (!OutputOrErr)
+    return OutputOrErr.takeError();
+  std::unique_ptr<FileOutputBuffer> Output = std::move(*OutputOrErr);
+  std::copy(getImage().bytes_begin(), getImage().bytes_end(),
+            Output->getBufferStart());
+  if (Error E = Output->commit())
+    return std::move(E);
+
+  return OffloadFile{TheImageKind,         TheOffloadKind,  getFlags(),
+                     getTripleStr().str(), getArch().str(), getFeatures().str(),
+                     getCmdLine().str(),   Filename.str()};
+}
+
+OffloadKind getOffloadKind(StringRef Name) {
+  return llvm::StringSwitch<OffloadKind>(Name)
+      .Case("openmp", OFK_OpenMP)
+      .Case("cuda", OFK_Cuda)
+      .Case("hip", OFK_HIP)
+      .Default(OFK_None);
+}
+
+StringRef getOffloadKindName(OffloadKind Kind) {
+  switch (Kind) {
+  case OFK_OpenMP:
+    return "openmp";
+  case OFK_Cuda:
+    return "cuda";
+  case OFK_HIP:
+    return "hip";
+  default:
+    return "none";
+  }
+}
+
+ImageKind getImageKind(StringRef Name) {
+  return llvm::StringSwitch<ImageKind>(Name)
+      .Case("o", IMG_Object)
+      .Case("bc", IMG_Bitcode)
+      .Case("cubin", IMG_Cubin)
+      .Case("fatbin", IMG_Fatbinary)
+      .Case("s", IMG_PTX)
+      .Default(IMG_None);
+}
+
+StringRef getImageKindName(ImageKind Kind) {
+  switch (Kind) {
+  case IMG_Object:
+    return "o";
+  case IMG_Bitcode:
+    return "bc";
+  case IMG_Cubin:
+    return "cubin";
+  case IMG_Fatbinary:
+    return "fatbin";
+  case IMG_PTX:
+    return "s";
+  default:
+    return "";
+  }
+}
+
+} // namespace clang

clang/lib/CodeGen/BackendUtil.cpp

 //===--- BackendUtil.cpp - LLVM Backend Utilities -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/Offloading.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"

 }
 
 void clang::EmbedObject(llvm::Module *M, const CodeGenOptions &CGOpts,
                         DiagnosticsEngine &Diags) {
   if (CGOpts.OffloadObjects.empty())
     return;
 
   for (StringRef OffloadObject : CGOpts.OffloadObjects) {
-    if (OffloadObject.count(',') != 1)
-      Diags.Report(Diags.getCustomDiagID(
-          DiagnosticsEngine::Error, "Invalid string pair for embedding '%0'"))
-          << OffloadObject;
-    auto FilenameAndSection = OffloadObject.split(',');
-    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> ObjectOrErr =
-        llvm::MemoryBuffer::getFileOrSTDIN(FilenameAndSection.first);
-    if (std::error_code EC = ObjectOrErr.getError()) {
-      auto DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
-                                          "could not open '%0' for embedding");
-      Diags.Report(DiagID) << FilenameAndSection.first;
+    StringMap<StringRef> Fields;
+    SmallVector<StringRef, 4> ObjectFields;
+    OffloadObject.split(ObjectFields, ',');
+
+    for (StringRef Field : ObjectFields) {
+      auto KeyAndValue = Field.split('=');
+      Fields[KeyAndValue.first] = KeyAndValue.second;
+    }
+
+    // Fill the offload file descriptor with the provided metadata. If this gets
+    // any more complicated it should be moved to its own clang tool.
+    OffloadFile File{};
+    File.Filename = Fields["file"].str();
+    File.TheOffloadKind = getOffloadKind(Fields["kind"]);
+    File.TheImageKind = getImageKind(Fields["file"].rsplit(".").second);
+    File.TheTriple = Fields["triple"].str();
+    File.Arch = Fields["arch"].str();
+    File.Features = Fields["features"].str();
+    File.CmdLine = Fields["cmdline"].str();
+
+    Expected<std::unique_ptr<MemoryBuffer>> BufferOrErr =
+        OffloadBinary::write(File);
+    if (!BufferOrErr) {
+      auto DiagID = Diags.getCustomDiagID(
+          DiagnosticsEngine::Error, "Could not embed offload object: '%0'");
+      Diags.Report(DiagID) << BufferOrErr.takeError();
       return;
     }
 
-    SmallString<128> SectionName(
-        {".llvm.offloading.", FilenameAndSection.second});
-    llvm::embedBufferInModule(*M, **ObjectOrErr, SectionName);
+    llvm::embedBufferInModule(*M, **BufferOrErr, ".llvm.offloading");
   }
 }

clang/lib/Driver/ToolChains/Clang.cpp

if (IsOpenMPHost && !OpenMPHostInputs.empty()) {
     auto InputFile = OpenMPHostInputs.begin();
     auto OpenMPTCs = C.getOffloadToolChains<Action::OFK_OpenMP>();
     for (auto TI = OpenMPTCs.first, TE = OpenMPTCs.second; TI != TE;
          ++TI, ++InputFile) {
       const ToolChain *TC = TI->second;
       const ArgList &TCArgs = C.getArgsForToolChain(TC, "", Action::OFK_OpenMP);
       StringRef File =
           C.getArgs().MakeArgString(TC->getInputFilename(*InputFile));
-      StringRef InputName = Clang::getBaseInputStem(Args, Inputs);
 
       CmdArgs.push_back(Args.MakeArgString(
-          "-fembed-offload-object=" + File + "," +
-          Action::GetOffloadKindName(Action::OFK_OpenMP) + "." +
-          TC->getTripleString() + "." +
-          TCArgs.getLastArgValue(options::OPT_march_EQ) + "." + InputName));
+          "-fembed-offload-object=file=" + File + "," +
+          "kind=" + Action::GetOffloadKindName(Action::OFK_OpenMP) + "," +
+          "triple=" + TC->getTripleString() + "," +
+          "arch=" + TCArgs.getLastArgValue(options::OPT_march_EQ)));
     }
   }
 
   if (Triple.isAMDGPU()) {
     handleAMDGPUCodeObjectVersionOptions(D, Args, CmdArgs);
 
     if (Args.hasFlag(options::OPT_munsafe_fp_atomics,
                      options::OPT_mno_unsafe_fp_atomics, /*Default=*/false))

clang/test/Driver/openmp-offload-gpu.c

 // NEW_DRIVER: "nvptx64-nvidia-cuda" - "clang", inputs: ["[[DEVICE_INPUT:.+]]", "[[HOST_BC]]"], output: "[[DEVICE_ASM:.+]]"
 // NEW_DRIVER: "nvptx64-nvidia-cuda" - "NVPTX::Assembler", inputs: ["[[DEVICE_ASM]]"], output: "[[DEVICE_OBJ:.+]]" 
 
 // RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target=nvptx64-nvida-cuda -march=sm_70 \
 // RUN:          --libomptarget-nvptx-bc-path=%S/Inputs/libomptarget/libomptarget-new-nvptx-test.bc \
 // RUN:          -fopenmp-new-driver -no-canonical-prefixes -nogpulib %s -o openmp-offload-gpu 2>&1 \
 // RUN:   | FileCheck -check-prefix=NEW_DRIVER_EMBEDDING %s
 
-// NEW_DRIVER_EMBEDDING: -fembed-offload-object=[[CUBIN:.*\.cubin]],openmp.nvptx64-nvidia-cuda.sm_70
+// NEW_DRIVER_EMBEDDING: -fembed-offload-object=file=[[CUBIN:.*\.cubin]],kind=openmp,triple=nvptx64-nvidia-cuda,arch=sm_70

clang/test/Frontend/embed-object.c

-// RUN: %clang_cc1 -x c -triple x86_64-unknown-linux-gnu -emit-llvm -fembed-offload-object=%S/Inputs/empty.h,section
+// RUN: %clang_cc1 -x c -triple x86_64-unknown-linux-gnu -emit-llvm -fembed-offload-object=file=%S/Inputs/empty.h -o - | FileCheck %s
 
-// CHECK: @[[OBJECT:.+]] = private constant [0 x i8] zeroinitializer, section ".llvm.offloading.section"
-// CHECK: @llvm.compiler.used = appending global [3 x i8*] [i8* getelementptr inbounds ([0 x i8], [0 x i8]* @[[OBJECT1]]], section "llvm.metadata"
+// CHECK: @[[OBJECT:.+]] = private constant [88 x i8] c"\10\FF\10\AD\01{{.*}}\00\00", section ".llvm.offloading"
+// CHECK: @llvm.compiler.used = appending global [1 x i8*] [i8* getelementptr inbounds ([88 x i8], [88 x i8]* @[[OBJECT]], i32 0, i32 0)], section "llvm.metadata"
 
 void foo(void) {}

clang/test/Frontend/embed-object.ll

 ; RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm \
-; RUN:    -fembed-offload-object=%S/Inputs/empty.h,section1 \
-; RUN:    -fembed-offload-object=%S/Inputs/empty.h,section2 -x ir %s -o - \
+; RUN:    -fembed-offload-object=file=%S/Inputs/empty.h -x ir %s -o - \
 ; RUN:    | FileCheck %s -check-prefix=CHECK
 
-; CHECK: @[[OBJECT1:.+]] = hidden constant [0 x i8] zeroinitializer, section ".llvm.offloading.section1"
-; CHECK: @[[OBJECT2:.+]] = hidden constant [0 x i8] zeroinitializer, section ".llvm.offloading.section2"
-; CHECK: @llvm.compiler.used = appending global [3 x i8*] [i8* @x, i8* getelementptr inbounds ([0 x i8], [0 x i8]* @[[OBJECT1]], i32 0, i32 0), i8* getelementptr inbounds ([0 x i8], [0 x i8]* @[[OBJECT2]], i32 0, i32 0)], section "llvm.metadata"
+; CHECK: @[[OBJECT:.+]] = private constant [88 x i8] c"\10\FF\10\AD{{.*}}\00", section ".llvm.offloading"
+; CHECK: @llvm.compiler.used = appending global [2 x i8*] [i8* @x, i8* getelementptr inbounds ([88 x i8], [88 x i8]* @[[OBJECT]], i32 0, i32 0)], section "llvm.metadata"
 
 @x = private constant i8 1
 @llvm.compiler.used = appending global [1 x i8*] [i8* @x], section "llvm.metadata"
 
 define i32 @foo() {
   ret i32 0
 }

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

 // linked device images for offloading. It scans the linker's input for embedded
 // device offloading data stored in sections `.llvm.offloading.<triple>.<arch>`
 // and extracts it as a temporary file. The extracted device files will then be
 // passed to a device linking job to create a final device image.
 //
 //===---------------------------------------------------------------------===//
 
 #include "OffloadWrapper.h"
+#include "clang/Basic/Offloading.h"
 #include "clang/Basic/Version.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
 #include "llvm/CodeGen/CommandFlags.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IRReader/IRReader.h"

 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 
 using namespace llvm;
+using namespace clang;
 using namespace llvm::object;
 
 static cl::opt<bool> Help("h", cl::desc("Alias for -help"), cl::Hidden);
 
 enum DebugKind {
   NoDebugInfo,
   DirectivesOnly,
   FullDebugInfo,

 
 /// Temporary files created by the linker wrapper.
 static SmallVector<std::string, 16> TempFiles;
 
 /// Codegen flags for LTO backend.
 static codegen::RegisterCodeGenFlags CodeGenFlags;
 
 /// Magic section string that marks the existence of offloading data. The
-/// section string will be formatted as `.llvm.offloading.<triple>.<arch>`.
-#define OFFLOAD_SECTION_MAGIC_STR ".llvm.offloading."
+/// section will contain one or more offloading binaries stored contiguously.
+#define OFFLOAD_SECTION_MAGIC_STR ".llvm.offloading"
 
+// TODO: Replace all uses of DeviceFile with OffloadFile.
 /// Information for a device offloading file extracted from the host.
 struct DeviceFile {
   DeviceFile(StringRef Kind, StringRef TheTriple, StringRef Arch,
              StringRef Filename)
       : Kind(Kind), TheTriple(TheTriple), Arch(Arch), Filename(Filename) {}
 
   std::string Kind;
   std::string TheTriple;
void printCommands(ArrayRef<StringRef> CmdArgs) {
   if (CmdArgs.empty())
     return;
 
   llvm::errs() << " \"" << CmdArgs.front() << "\" ";
   for (auto IC = std::next(CmdArgs.begin()), IE = CmdArgs.end(); IC != IE; ++IC)
     llvm::errs() << *IC << (std::next(IC) != IE ? " " : "\n");
 }
 
-static StringRef getDeviceFileExtension(StringRef DeviceTriple,
-                                        bool IsBitcode = false) {
-  Triple TheTriple(DeviceTriple);
-  if (TheTriple.isAMDGPU() || IsBitcode)
-    return "bc";
-  if (TheTriple.isNVPTX())
-    return "cubin";
-  return "o";
-}
-
 std::string getMainExecutable(const char *Name) {
   void *Ptr = (void *)(intptr_t)&getMainExecutable;
   auto COWPath = sys::fs::getMainExecutable(Name, Ptr);
   return sys::path::parent_path(COWPath).str();
 }
 
 /// Extract the device file from the string '<kind>-<triple>-<arch>=<library>'.
 DeviceFile getBitcodeLibrary(StringRef LibraryStr) {

 
 Expected<Optional<std::string>>
 extractFromBinary(const ObjectFile &Obj,
                   SmallVectorImpl<DeviceFile> &DeviceFiles) {
   StringRef Extension = sys::path::extension(Obj.getFileName()).drop_front();
   StringRef Prefix = sys::path::stem(Obj.getFileName());
   SmallVector<StringRef, 4> ToBeStripped;
 
-  // Extract data from sections of the form `.llvm.offloading.<triple>.<arch>`.
+  // Extract offloading binaries from sections with the name `.llvm.offloading`.
   for (const SectionRef &Sec : Obj.sections()) {
     Expected<StringRef> Name = Sec.getName();
-    if (!Name || !Name->startswith(OFFLOAD_SECTION_MAGIC_STR))
+    if (!Name || !Name->equals(OFFLOAD_SECTION_MAGIC_STR))
       continue;
 
-    SmallVector<StringRef, 4> SectionFields;
-    Name->split(SectionFields, '.');
-    StringRef Kind = SectionFields[3];
-    StringRef DeviceTriple = SectionFields[4];
-    StringRef Arch = SectionFields[5];
+    Expected<StringRef> Contents = Sec.getContents();
+    if (!Contents)
+      return Contents.takeError();
+
+    uint64_t Offset = 0;
+    // There could be multiple offloading binaries stored at this section.
+    while (Offset < Contents->size()) {
+      std::unique_ptr<MemoryBuffer> Buffer =
+          MemoryBuffer::getMemBuffer(Contents->drop_front(Offset), *Name,
+                                     /*RequiresNullTerminator*/ false);
+      auto BinaryOrErr = OffloadBinary::create(*Buffer);
+      if (!BinaryOrErr)
+        return BinaryOrErr.takeError();
+      OffloadBinary &Binary = **BinaryOrErr;
+
+      StringRef Kind =
+          getOffloadKindName(static_cast<OffloadKind>(Binary.getOffloadKind()));
+      StringRef Suffix =
+          getImageKindName(static_cast<ImageKind>(Binary.getImageKind()));
 
-    if (Expected<StringRef> Contents = Sec.getContents()) {
       SmallString<128> TempFile;
-      StringRef DeviceExtension = getDeviceFileExtension(
-          DeviceTriple, identify_magic(*Contents) == file_magic::bitcode);
       if (Error Err = createOutputFile(Prefix + "-" + Kind + "-" +
-                                           DeviceTriple + "-" + Arch,
-                                       DeviceExtension, TempFile))
+                                           Binary.getTripleStr() + "-" +
+                                           Binary.getArch(),
+                                       Suffix, TempFile))
         return std::move(Err);
 
-      Expected<std::unique_ptr<FileOutputBuffer>> OutputOrErr =
-          FileOutputBuffer::create(TempFile, Sec.getSize());
-      if (!OutputOrErr)
-        return OutputOrErr.takeError();
-      std::unique_ptr<FileOutputBuffer> Output = std::move(*OutputOrErr);
-      std::copy(Contents->begin(), Contents->end(), Output->getBufferStart());
-      if (Error E = Output->commit())
-        return std::move(E);
+      Expected<OffloadFile> File = Binary.createOffloadFile(TempFile);
+      if (!File)
+        return File.takeError();
 
-      DeviceFiles.emplace_back(Kind, DeviceTriple, Arch, TempFile);
-      ToBeStripped.push_back(*Name);
+      DeviceFiles.emplace_back(Kind, File->TheTriple, File->Arch,
+                               File->Filename);
+
+      Offset += Binary.getSize();
     }
+
+    ToBeStripped.push_back(*Name);
   }
 
   if (ToBeStripped.empty() || !StripSections)
     return None;
 
   // If the object file to strip doesn't exist we need to write it so we can
   // pass it to llvm-strip.
   SmallString<128> StripFile = Obj.getFileName();
return createStringError(inconvertibleErrorCode(),
                              "Failed to create module");
 
   StringRef Extension = sys::path::extension(M->getName()).drop_front();
   StringRef Prefix =
       sys::path::stem(M->getName()).take_until([](char C) { return C == '-'; });
 
   SmallVector<GlobalVariable *, 4> ToBeDeleted;
 
-  // Extract data from the global string containing a section of the form
-  // `.llvm.offloading.<triple>.<arch>`.
+  // Extract offloading data from globals with the `.llvm.offloading` section
+  // name.
   for (GlobalVariable &GV : M->globals()) {
-    if (!GV.hasSection() ||
-        !GV.getSection().startswith(OFFLOAD_SECTION_MAGIC_STR))
+    if (!GV.hasSection() || !GV.getSection().equals(OFFLOAD_SECTION_MAGIC_STR))
       continue;
 
     auto *CDS = dyn_cast<ConstantDataSequential>(GV.getInitializer());
     if (!CDS)
       continue;
 
-    SmallVector<StringRef, 4> SectionFields;
-    GV.getSection().split(SectionFields, '.');
-    StringRef Kind = SectionFields[3];
-    StringRef DeviceTriple = SectionFields[4];
-    StringRef Arch = SectionFields[5];
-
     StringRef Contents = CDS->getAsString();
+
+    uint64_t Offset = 0;
+    // There could be multiple offloading binaries stored at this section.
+    while (Offset < Contents.size()) {
+      std::unique_ptr<MemoryBuffer> Buffer =
+          MemoryBuffer::getMemBuffer(Contents.drop_front(Offset), GV.getName(),
+                                     /*RequiresNullTerminator*/ false);
+      auto BinaryOrErr = OffloadBinary::create(*Buffer);
+      if (!BinaryOrErr)
+        return BinaryOrErr.takeError();
+      OffloadBinary &Binary = **BinaryOrErr;
+
+      StringRef Kind =
+          getOffloadKindName(static_cast<OffloadKind>(Binary.getOffloadKind()));
+      StringRef Suffix =
+          getImageKindName(static_cast<ImageKind>(Binary.getImageKind()));
+
       SmallString<128> TempFile;
-    StringRef DeviceExtension = getDeviceFileExtension(
-        DeviceTriple, identify_magic(Contents) == file_magic::bitcode);
-    if (Error Err = createOutputFile(Prefix + "-" + Kind + "-" + DeviceTriple +
-                                         "-" + Arch,
-                                     DeviceExtension, TempFile))
+      if (Error Err = createOutputFile(Prefix + "-" + Kind + "-" +
+                                           Binary.getTripleStr() + "-" +
+                                           Binary.getArch(),
+                                       Suffix, TempFile))
         return std::move(Err);
 
-    Expected<std::unique_ptr<FileOutputBuffer>> OutputOrErr =
-        FileOutputBuffer::create(TempFile, Contents.size());
-    if (!OutputOrErr)
-      return OutputOrErr.takeError();
-    std::unique_ptr<FileOutputBuffer> Output = std::move(*OutputOrErr);
-    std::copy(Contents.begin(), Contents.end(), Output->getBufferStart());
-    if (Error E = Output->commit())
-      return std::move(E);
+      Expected<OffloadFile> File = Binary.createOffloadFile(TempFile);
+      if (!File)
+        return File.takeError();
+
+      DeviceFiles.emplace_back(Kind, File->TheTriple, File->Arch,
+                               File->Filename);
 
-    DeviceFiles.emplace_back(Kind, DeviceTriple, Arch, TempFile);
+      Offset += Binary.getSize();
+    }
     ToBeDeleted.push_back(&GV);
   }
 
   if (ToBeDeleted.empty() || !StripSections)
     return None;
 
   // We need to materialize the lazy module before we make any changes.
   if (Error Err = M->materializeAll())

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

static SectionKind getELFKindForNamedSection(StringRef Name, SectionKind K) {
   //
   //   .section   .eh_frame,"a",@progbits
 
   if (Name == getInstrProfSectionName(IPSK_covmap, Triple::ELF,
                                       /*AddSegmentInfo=*/false) ||
       Name == getInstrProfSectionName(IPSK_covfun, Triple::ELF,
                                       /*AddSegmentInfo=*/false) ||
       Name == ".llvmbc" || Name == ".llvmcmd" ||
-      Name.startswith(".llvm.offloading."))
+      Name.startswith(".llvm.offloading"))
     return SectionKind::getMetadata();
 
   if (Name.empty() || Name[0] != '.') return K;
 
   // Default implementation based on some magic section names.
   if (Name == ".bss" ||
       Name.startswith(".bss.") ||
       Name.startswith(".gnu.linkonce.b.") ||

llvm/lib/Transforms/Utils/ModuleUtils.cpp

#ifndef NDEBUG
   }
 #endif
   CI->addFnAttr(
       Attribute::get(M->getContext(), MappingsAttrName, Buffer.str()));
 }
 
 void llvm::embedBufferInModule(Module &M, MemoryBufferRef Buf,
                                StringRef SectionName) {
-  // Embed the buffer into the module.
+  // Embed the memory buffer into the module.
   Constant *ModuleConstant = ConstantDataArray::get(
       M.getContext(), makeArrayRef(Buf.getBufferStart(), Buf.getBufferSize()));
   GlobalVariable *GV = new GlobalVariable(
-      M, ModuleConstant->getType(), true, GlobalValue::ExternalLinkage,
-      ModuleConstant, SectionName.drop_front());
+      M, ModuleConstant->getType(), true, GlobalValue::PrivateLinkage,
+      ModuleConstant, "llvm.embedded.object");
   GV->setSection(SectionName);
-  GV->setVisibility(GlobalValue::HiddenVisibility);
 
   appendToCompilerUsed(M, GV);
 }