jhuber6 added a comment.
Thanks for the comments.
================
Comment at: clang/test/Driver/linker-wrapper.c:109
// RUN: clang-offload-packager -o %t-lib.out \
// RUN:
--image=file=%S/Inputs/dummy-elf.o,kind=openmp,triple=nvptx64-nvidia-cuda,arch=sm_70
\
----------------
tra wrote:
> Nit: This test case does not have any CHECK lines and could use a comment
> describing what it's supposed to test. AFAICT it's intended to make sure that
> no temporary files are left around, but I'm not 100% sure.
Yes, it ensures that the files extracted from the static library are not
leftover as temp files, this was a problem previously that I fixed. I'll add a
comment explaining that.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:620-622
+ CmdArgs.push_back("-bundle-align=4096");
+
+ SmallVector<StringRef> Targets = {"-targets=host-x86_64-unknown-linux"};
----------------
tra wrote:
> We probably do not want to hardcode the assumption that the host is x86_64
> linux.
>
> Bundle alignment should probably also be target-dependent, but 4K is common
> enough and is probably fine in practice.
This is exactly the way it is in the Clang source for HIP. HIP uses the
`clang-offload-bundler` which expects a host file and host triple, ergo the
dummy triple and input from `/dev/null`. This obviously isn't great, maybe in
the future I'll be able to convince the AMD folks to use my format instead.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1205
+bundleHIP(ArrayRef<OffloadingImage> Images) {
+ SmallVector<std::unique_ptr<MemoryBuffer>> Buffers;
+
----------------
tra wrote:
> I'd move it to the end where the buffer is actually used.
Sure, I'll do that for the others as well.
================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:393
+ /*Initializer*/ nullptr,
+ IsHIP ? "__start_hip_offloading_entries"
+ : "__start_cuda_offloading_entries");
----------------
tra wrote:
> We should probably have a helper function returning properly prefixed name,
> similar to what we do in clang:
> https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGCUDANV.cpp#L184
I had that thought, but unless I wanted to use regular expressions it would be
a little weird since there's many different types here, e.g. `__cuda`, `.cuda`
and `_cuda`. I figured it was easier to just make two strings rather than carry
around three different functions to handle these cases, or introduce some
weird regex.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128914/new/
https://reviews.llvm.org/D128914
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
