[PATCH] D129507: [OffloadPackager] Add option to extract files from images

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/test/Driver/offload-packager.c:12-14
+// RUN: clang-offload-packager %t.out \
+// RUN:   --image=file=%t-sm_70.o,kind=openmp,triple=nvptx64-nvidia-cuda,arch=sm_70 \
+// RUN:   --image=file=%t-gfx908.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx908
----------------
So, with no `-o` option the tool treats `--image` to both choose the embedded image to dump and the file name to dump it into.
The patch appears to have more dumping modes. E.g. file name appears to be deriveable from the embedded image info. That should be tested, too.

Does user need to specify complete kind/triple/arch? Can I dump a subset of images -- e.g. all images with the same triple, or all with `arch` starting with `sm_7`?

It would also be very convenient if we could dump an image by its index in the executable and, also, all images. I suspect these two modes would be the ones used most often interactively.

BTW, we do not seem to have a way to list those embedded images. It would be great to add it.


================
Comment at: clang/test/Driver/offload-packager.c:15
+// RUN:   --image=file=%t-gfx908.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx908
+// RUN: diff %t-sm_70.o %S/Inputs/dummy-elf.o
+// RUN: diff %t-gfx908.o %S/Inputs/dummy-elf.o
----------------
I'd use `diff -q` or maybe `cmp`, though I don't know if it's available on Windows. We only want to know if the files differ and do not want it to accidentally dump binary on the screen.



================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:70
     for (StringRef Arg : llvm::split(Image, ",")) {
-      auto KeyAndValue = Arg.split("=");
-      if (Args.count(KeyAndValue.first))
-        Args[KeyAndValue.first] =
-            Saver.save(Args[KeyAndValue.first] + "," + KeyAndValue.second);
+      auto [Key, Value] = Arg.split("=");
+      if (Args.count(Key))
----------------
Hooray for c++17!



================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:168
+
+    DenseMap<StringRef, StringRef> Args;
+    for (StringRef Arg : llvm::split(Image, ",")) {
----------------
A comment would be useful here. Or conversion of `--image=k1=v1,k2=v2,k3=v3...` into a `kN->vN` map could be extracted into a helper function with a meaningful name.


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:193
+      StringRef Filename =
+          !Args.count("file")
+              ? Saver.save(sys::path::stem(InputFile) + "-" +
----------------
Nit: `Args.count("file") ? Args["file"] : Saver.save(...)` has one less `!`.


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:194-196
+              ? Saver.save(sys::path::stem(InputFile) + "-" +
+                           Binary->getTriple() + "-" + Binary->getArch() + "." +
+                           getImageKindName(Binary->getImageKind()))
----------------
Is this guaranteed to be unique? I'd add an image index to the name. 


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:204
+      std::unique_ptr<FileOutputBuffer> Output = std::move(*OutputOrErr);
+      std::copy(Binary->getImage().bytes_begin(),
+                Binary->getImage().bytes_end(), Output->getBufferStart());
----------------
You could use `llvm::copy` which works with ranges.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129507