[PATCH] D128914: [HIP] Add support for handling HIP in the linker wrapper

Joseph Huber via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 30 13:31:21 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list