[Openmp-commits] [PATCH] D127432: [Libomptarget] Add support for offloading binaries in libomptarget

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added a comment.

In D127432#3576385 <https://reviews.llvm.org/D127432#3576385>, @tianshilei1992 wrote:

> Is it necessary to create `__tgt_image_info` in `libomptarget`? If the front end emits a binary compatible with `OffloadBinary`, all the information a plugin needs is in it right? Can we just include the header and read those information in plugin instead of in `libomptarget` such that we don't need to change the interface?

I was originally thinking about doing that, but it's much noisier since we'd need to change a lot of code that actually loads the binaries. It would free us from reallocating these structs however. Otherwise we'd need to change each usage of the Image, this just extracts the image at the start and everything else stays the same. I don't know which is the superior approach. I find this is a "cleaner" way to augment the existing functionality while maintaining backwards compatibility, but maybe there's a need for everything else to have access to the metadata in the plugin.



================
Comment at: openmp/libomptarget/include/OffloadBinary.h:71
+  uint64_t getImageSize() const { return TheEntry->ImageSize; }
+  const char *getString(const char *Key) const {
+    return (StringData.count(Key)) ? StringData.at(Key) : "";
----------------
tianshilei1992 wrote:
> you may want to make it private
It doesn't modify any content so 


================
Comment at: openmp/libomptarget/include/OffloadBinary.h:72
+  const char *getString(const char *Key) const {
+    return (StringData.count(Key)) ? StringData.at(Key) : "";
+  }
----------------
tianshilei1992 wrote:
> Does it make more sense to return `nullptr` instead of `""` here?
It's supposed to be a string, so an empty string is more appropriate than a null pointer I feel.


================
Comment at: openmp/libomptarget/include/OffloadBinary.h:75
+
+  OffloadBinary(char *Buffer, const Header *TheHeader, const Entry *TheEntry)
+      : Buffer(Buffer), TheHeader(TheHeader), TheEntry(TheEntry) {
----------------
tianshilei1992 wrote:
> you may want to make it private
It should be private, I'll see if I can do that considering the static create functions calls it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127432/new/

https://reviews.llvm.org/D127432