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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/docs/ClangOffloadBinary.rst:8
+
+.. _clang-offload-binary:
+
----------------
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.


================
Comment at: clang/docs/ClangOffloadBinary.rst:42
+  
+  clang-offload-binary options:
+  
----------------
This appears to be a one-way process. How one would examine what's in the binary and unpack/extract specific component from it?


================
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"
----------------
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.


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


================
Comment at: clang/tools/clang-offload-binary/ClangOffloadBinary.cpp:75
+
+    if (!Args.count("triple") || !Args.count("file"))
+      return reportError(createStringError(
----------------
Should `kind` also be required? If not, what's the default kind?


================
Comment at: clang/tools/clang-offload-binary/ClangOffloadBinary.cpp:99
+    }
+    std::unique_ptr<MemoryBuffer> Buffer = OffloadBinary::write(ImageBinary);
+    OS << Buffer->getBuffer();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125165