jhuber6 marked 2 inline comments as done. jhuber6 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 ---------------- tra wrote: > 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. > I think we need something like ar --output=dir which is used for very similar > purposes. That will work for now, I'll add that and update this patch with better testing. > Now that clang-offload-packager got more functionality, it would make sense > to make it the tool for listing embedded images, too, IMO. It shouldn't be too difficult to add since we'll be iterating through all of these already. I could do that in another patch. ================ 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: > 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. > > Yeah, I'll add `.x` to the filename for that reason. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129507/new/ https://reviews.llvm.org/D129507 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
