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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked 3 inline comments as done.
jhuber6 added a comment.

Thanks for the 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
----------------
tra wrote:
> 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.
> 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 derivable from the embedded image info. That should be tested, too.
I tried to write a test for this. Given no explicit filename and multiple matches it should dump it as `<input>-<triple>-<arch>.o` or similar. It works when I test it locally but when I wrote a test for this I don't think it was writing to the correct directory because the `lit` test couldn't find them.
> 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?
>
As written now, it should just write every image it finds that matches the image values. So `--image=kind=openmp` would dump everything whose producer was OpenMP. If it was `--image=kind=openmp,file=out.o` it would then write to `out.o` multiple times so you'd just end up with the last one. If you didn't provide a filename it would derive one as above. Not sure if it's worth handling this more intelligently since I think we should keep the user's requested name even if it's ambiguous.
> 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.

That should be was `llvm-objdump --offloading` does, right? Or do you mean something different from just printing what's embedded.


================
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))
----------------
tra wrote:
> Hooray for c++17!
> 
Indeed.


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:168
+
+    DenseMap<StringRef, StringRef> Args;
+    for (StringRef Arg : llvm::split(Image, ",")) {
----------------
tra wrote:
> 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.
I was thinking about that as it's somewhat obtuse, I'll try to make it clearer.


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:193
+      StringRef Filename =
+          !Args.count("file")
+              ? Saver.save(sys::path::stem(InputFile) + "-" +
----------------
tra wrote:
> Nit: `Args.count("file") ? Args["file"] : Saver.save(...)` has one less `!`.
The formatting looked a little weird when I did it that way. Not a huge deal however.


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:194-196
+              ? Saver.save(sys::path::stem(InputFile) + "-" +
+                           Binary->getTriple() + "-" + Binary->getArch() + "." +
+                           getImageKindName(Binary->getImageKind()))
----------------
tra wrote:
> Is this guaranteed to be unique? I'd add an image index to the name. 
Not necessarily, you're right that it might be good to use the image's index to provide a unique name in the case of conflicts. As mentioned in another comment I'm not sure if we should modify the output file if the user specified it however.


================
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());
----------------
tra wrote:
> You could use `llvm::copy` which works with ranges.
Good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129507