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

[Artem Belevich via Phabricator via llvm-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
----------------
jhuber6 wrote:
> 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.
> lit test couldn't find them.

I see. The directory the test is run from is not necessarily the temporary directory. For that matter, such a test will fail in some setups where the source tree is read-only.
I think we need something like `ar --output=dir` which is used for very similar purposes.

>> Can I dump a subset of images ?

Given that we can narrow the subset to particular kind/triple/arch, it should do for now.

> That should be was llvm-objdump --offloading does, right? 

Now that `lang-offload-packager` got more functionality, it would make sense to make it the tool for listing embedded images, too, IMO. 


================
Comment at: clang/tools/clang-offload-packager/ClangOffloadPackager.cpp:194-196
+              ? Saver.save(sys::path::stem(InputFile) + "-" +
+                           Binary->getTriple() + "-" + Binary->getArch() + "." +
+                           getImageKindName(Binary->getImageKind()))
----------------
jhuber6 wrote:
> 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.
Agreed, user input takes precedence. 

My concern was for the clashes caused by the tool itself. There may conceivably be embedded images with identical properties and we need a way to distinguish between them.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129507