[PATCH] D124525: [OpenMP][ClangLinkerWrapper] Extending linker wrapper to embed metadata for multi-arch fat binaries

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:246
   IRBuilder<> Builder(BasicBlock::Create(C, "entry", Func));
+  // Create calls to __tgt_register_image_info for each image
+  auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
----------------
saiislam wrote:
> jhuber6 wrote:
> > I'm wondering if it would be better to create a new `__tgt_bin_desc` and call a new `__tgt_register_lib` with it here so we don't need multiple calls here. Inside that new runtime function we could just widen or shrink the existing structs as needed. That way each device image would have this metadata associated with it and the target plugin can handle it as-needed.
> Last time multiple vendors objected to changing `__tgt_bin_desc` and `__tgt_device_image` structs. The reason was backward compatibility of multiple downstream runtimes.
If you keep the old `__tgt_register_lib` function, old applications will use that. Then inside libomptarget you can change `__tgt_register_lib` function to create a new `__tgt_device_image` from the old format. It would have some time penalty as we'd need to allocate some new structs, but it might be easier to deal with if each image contained its own metadata for the plugin to use. I was imagining it would look something like
```
struct __tgt_device_image_v2 {
  void* 	ImageStart;
  void* 	ImageEnd;
  __tgt_offload_entry* 	EntriesBegin;
  __tgt_offload_entry* 	EntriesEnd;
  __tgt_image_into *  ImageInfo;
};
```

Not sure if this is the best approach, but it should let us keep backwards compatibility and change the structs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124525