[PATCH] D110549: [HIPSPV][1/4] Refactor HIP tool chain

Mon Nov 22 15:27:34 PST 2021

yaxunl added inline comments.

================
Comment at: clang/lib/Driver/ToolChains/HIPUtility.cpp:119-133
+  // Add MC directives to embed target binaries. We ensure that each
+  // section and image is 16-byte aligned. This is not mandatory, but
+  // increases the likelihood of data to be aligned with a cache block
+  // in several main host machines.
+  ObjStream << "#       HIP Object Generator\n";
+  ObjStream << "# *** Automatically generated by Clang ***\n";
+  ObjStream << "  .protected __hip_fatbin\n";
----------------
echristo wrote:
> tra wrote:
> > Using MC for just wrapping a blob into an object file strikes me as something MC is not intended for.
> > 
> > @echristo -- is it OK to use MC as a tool in the standard compilation pipeline? I vaguely recall we had a conversaion about using objcopy for similar purposes during early days of CUDA and the conclusion was that we generally don't want that. I might be wrong, too, it's been too long ago.
> Using llvm-mc the binary? No. That's definitely not something that should be done. And as far as objcopy we spent quite a bit of time pulling it out for split dwarf and, if possible, it shouldn't be used there either.
HIP toolchain has used llvm-mc to embed fat binary in a host object for a long time.

This is for supporting -fgpu-rdc since separated compilation/link will result in host object and device bit code. After device bit code are linked and fat binary are generated, fat binary needs to be embedded in some host object and linked with other host objects.

In the beginning, we used linker script. That works until we need to support windows. Then we adopted this suggestion (https://reviews.llvm.org/D46472#inline-412796) and use llvm-mc to generate a host object containing the fat binary, which works for both Linux and Windows.

I am wondering what is wrong with this approach and what is a better approach.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110549