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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added a comment.

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?



================
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) : "";
----------------
you may want to make it private


================
Comment at: openmp/libomptarget/include/OffloadBinary.h:72
+  const char *getString(const char *Key) const {
+    return (StringData.count(Key)) ? StringData.at(Key) : "";
+  }
----------------
Does it make more sense to return `nullptr` instead of `""` here?


================
Comment at: openmp/libomptarget/include/OffloadBinary.h:75
+
+  OffloadBinary(char *Buffer, const Header *TheHeader, const Entry *TheEntry)
+      : Buffer(Buffer), TheHeader(TheHeader), TheEntry(TheEntry) {
----------------
you may want to make it private


================
Comment at: openmp/libomptarget/src/rtl.cpp:13-16
 #include "rtl.h"
+#include "OffloadBinary.h"
 #include "device.h"
 #include "private.h"
----------------
I'd do the following.


================
Comment at: openmp/libomptarget/src/rtl.cpp:287-288
+
+  return __tgt_device_image{Binary->getImageStart(), Binary->getImageEnd(),
+                            Image->EntriesBegin, Image->EntriesEnd};
+}
----------------
plus `clang-format` of course.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127432