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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Syntax/style looks OK to me with a few nits.



================
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 \
----------------
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.


================
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"};
----------------
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.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1205
+bundleHIP(ArrayRef<OffloadingImage> Images) {
+  SmallVector<std::unique_ptr<MemoryBuffer>> Buffers;
+
----------------
I'd move it to the end where the buffer is actually used. 


================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:393
+                         /*Initializer*/ nullptr,
+                         IsHIP ? "__start_hip_offloading_entries"
+                               : "__start_cuda_offloading_entries");
----------------
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128914/new/

https://reviews.llvm.org/D128914