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

[Joseph Huber via Phabricator via cfe-commits]

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