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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
Comment at: openmp/libomptarget/include/omptarget.h:143
+  __tgt_image_info Info;             // Struct containing metadata
+};
+
----------------
jhuber6 wrote:
> tianshilei1992 wrote:
> > Is it better to define it as follows?
> > ```
> > struct __tgt_device_binary {
> >   __tgt_device_image Image;
> >   __tgt_image_info Info;
> > }
> > ```
> I did it that way to somewhat reduce noise when changing to the new format. But now that I'm thinking about it, where else will we need this metadata? It might be fine to just parse it out and create new images so we don't change them, e.g.
> ```
> __tgt_image_info info = getInfoFromImage(image); // Gets the strings out from the offload binary type in the __tgt_device_image`.
> __tgt_device_image image = getELFFromImage(image); //Gets the ELF out and makes a new image with begin / end set correctly (Need to make a new one, the pointers are in RO memory).
> is_binary_valid(image, info); // Plugin uses info struct if necessary, every other interface is the same.
> ```
> WDYT?
That looks neater.
In addition, we also want to have a clear definition for front end as well. I'm not saying the front end is gonna include the header. At least we have a clear definition with proper documentation, instead of nested definition in a function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127432