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

Artem Belevich via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 30 12:37:00 PDT 2022

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:

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list