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