[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> The tool indeed does not have anything specific to OpenMP at this step, but that will change...

That makes sense to me, thanks.

I think we're going to have some trouble adapting this to our build as there's already a standalone tool that runs at link time. Overall dropping the linker script is probably worth the integration headache.



================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:84
+  void createImages(ArrayRef<BinaryDesc> Binaries) {
+    for (const BinaryDesc &Bin : Binaries) {
+      StringRef SectionName = SS.save(".omp_offloading." + Bin.second);
----------------
sdmitriev wrote:
> JonChesterfield wrote:
> > I don't think this works for multiple binaries with the same target triple. They'll all be put in the same section and there will be duplicate symbols for start/end.
> Adding the same target triple to the list of OpenMP targets more than once is not supported, so such use case isn't viable:
> 
> ```
> bash-4.2$ clang -fopenmp -fopenmp-targets=x86_64-pc-linux-gnu,x86_64-pc-linux-gnu test.c
> clang-10: warning: The OpenMP offloading target 'x86_64-pc-linux-gnu' is similar to target 'x86_64-pc-linux-gnu' already specified - will be ignored. [-Wopenmp-target]
> bash-4.2$ 
> ```
> 
> But in any case I am going to remove the code which passes offload target triples to the wrapper tool in the last part of D64943 because they will not be needed for creating wrapper bit-code. As you know start/end symbols are referenced from the offload registration code only, so, moving offload registration code to the wrapper bit-code eliminates the need to create global start/end symbols with predefined names derived from the triple.
That's true. It seems a shame that we can embed at most one device binary per architecture into the host, but that's an existing limitation.


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

https://reviews.llvm.org/D68166