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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
