[PATCH] D125165: [Clang] Introduce clang-offload-binary tool to bundle device files

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

Thanks for the comments.



================
Comment at: clang/docs/ClangOffloadBinary.rst:8
+
+.. _clang-offload-binary:
+
----------------
tra wrote:
> Naming nit: `binary` may not be the best term for what we're trying to do here. Perhaps something like `package`, `container` or `collection` would work better.
Yeah, I would've used `bundler` but that name is taken. I think I can go with `clang-offload-container`


================
Comment at: clang/docs/ClangOffloadBinary.rst:42
+  
+  clang-offload-binary options:
+  
----------------
tra wrote:
> This appears to be a one-way process. How one would examine what's in the binary and unpack/extract specific component from it?
This is done by the linker wrapper, but I think it would be good to teach `llvm-objdump` how to handle these. Then we could basically just treat it the same way as `cuobjdump`.


================
Comment at: clang/test/Frontend/embed-object.ll:7
+; CHECK: @[[OBJECT_1:.+]] = private constant [0 x i8] zeroinitializer, section ".llvm.offloading", align 8
+; CHECK: @[[OBJECT_2:.+]] = private constant [0 x i8] zeroinitializer, section ".llvm.offloading", align 8
 ; CHECK: @llvm.compiler.used = appending global [3 x ptr] [ptr @x, ptr @[[OBJECT_1]], ptr @[[OBJECT_2]]], section "llvm.metadata"
----------------
tra wrote:
> What will happen if an openMP file compiled this way is linked with the older version of OpenMP runtime which presumably expected to see extra data in `.llvm.offloading`?
> Will it provide a sensible error? Perhaps we should change the section name, too.
I didn't change the actual data being embedded, only the method to do it. previously this command line did the work of the offload binary tool. Now it just embeds the file that the tool spits out. This test just makes sure that we run it and get the contents in the IR.


================
Comment at: clang/tools/clang-offload-binary/ClangOffloadBinary.cpp:70
+  raw_svector_ostream OS(BinaryData);
+  for (StringRef Image : DeviceImages) {
+    StringMap<StringRef> Args;
----------------
tra wrote:
> It would be useful to add a comment describing the 'special' keys `file` and `kind`.
I think I'll add that to the help message.


================
Comment at: clang/tools/clang-offload-binary/ClangOffloadBinary.cpp:75
+
+    if (!Args.count("triple") || !Args.count("file"))
+      return reportError(createStringError(
----------------
tra wrote:
> Should `kind` also be required? If not, what's the default kind?
The default kind is filled when we default construct the `OffloadingImage` below, which gives `OFK_None`. This has the effect of being used for linking jobs, but not emitting any registration code.


================
Comment at: clang/tools/clang-offload-binary/ClangOffloadBinary.cpp:99
+    }
+    std::unique_ptr<MemoryBuffer> Buffer = OffloadBinary::write(ImageBinary);
+    OS << Buffer->getBuffer();
----------------
tra wrote:
> Nit: `write` is a rather misleading function name here. AFAICT, we're not actually writing anything, but rather packing the `ImageBinary` into a memory buffer, which we then return.
I read it as "write to buffer", but I can see your point. It's not related to this patch but I could see changing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125165