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

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

In D68166#1692071 <https://reviews.llvm.org/D68166#1692071>, @JonChesterfield wrote:

> I think this patch is a behaviour change. Currently, the target binary is embedded in the host binary at link time. With this change, the contents of the binary are embedded in bitcode which is subsequently fed into the link. If indeed so, that seems strictly better - code in the host that cares about the size of the bitcode now has it available at opt time, instead of at link time. The target specific nastiness objcopy would introduce is neatly sidestepped.
>
> This change takes N binaries (that I think need to be for different triples, or the loop doesn't work) and puts them in separate section-annotated bitcode arrays. Equivalent behaviour would result from calling the tool once per binary and passing the N results onward, e.g. to llvm-link.
>
> The functionality of 'take a binary and embed it in bitcode as a const array' is likely to be useful outside of openmp. I've done similar things in the past in non-portable fashion. Aside from the section and symbol names, I don't think there's anything specific to openmp in the tool.
>
> How would you feel about simplifying the tool to work on one file at a time, with an interface that takes the host target (could default to whatever is running the tool) and a string for section name, which generates some bitcode containing that file as a const array plus start/end symbols derived from the section name? The change would involve deleting the multiple file handling and renaming OffloadTargets to SectionName or similar.
>
> clang-offload-wrapper than becomes binary-to-bitcode-embedder (or better, names are hard), with the intent that projects outside of the openmp target offload compiler could use it.
>
> edit: Or keep the multiple file handling if you prefer, preferably raising an error if there are duplicates in the requested section names


The tool indeed does not have anything specific to OpenMP at this step, but that will change in the 3rd part of the D64943 <https://reviews.llvm.org/D64943> where I am planning to move offload registration code generation from clang to the wrapper tool. So it will have OpenMP specifics in future, though I do not see any problems with enabling it for other offloading models. We can always change driver to pass an additional information that represent 'offload kind' to the wrapper tool (can for example be done in a similar way how it is passed to the bundler tool), and wrapper will customize output bit-code depending on the offloading model if there would be a need for that.

Regarding the multiple vs single file handling. Wrapping each device binary independently would still be possible with multi-file wrapping support, but it will just increase startup time without adding any benefits in return (once we move offload registration code to the wrapper). So, I think that for OpenMP it does not make sense to do it (I cannot say anything about the other offloading models though).

Anyway, I suggest so start with something that eliminates OpenMP linker script. We can always customize/improve tools in future once there would be a need for that.



================
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);
----------------
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.


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

https://reviews.llvm.org/D68166