[PATCH] D108291: [clang-nvlink-wrapper] Wrapper around nvlink for archive files

Wed Aug 18 05:16:45 PDT 2021

saiislam added a comment.

In D108291#2951969 <https://reviews.llvm.org/D108291#2951969>, @JonChesterfield wrote:

> Tests. The usual bug in this area is that an archive can contain multiple files with the same name which clobber each other if extracted to the same directory. It looks to me like this implementation will fail that test.

While extracting cubin files from the archive each output file gets a new name using llvm::sys::fs::createTemporaryFile, so there won't be any clobbering.

> Also, motivation? Seems this can be worked around by not putting cubin files in an archive.

It is required for linking static device libraries on nvptx (D105191 <https://reviews.llvm.org/D105191>). Given a fat heterogenous archive, clang-offload-bundler will create a device specific archive which will be passed to llvm-link for amdgpu and nvlink-wrapper for nvptx.

> Edit: this will fail to implement the archive semantics of only pulling in used files, i.e. it is implicitly -whole-archive. Suggest error unless whole-archive is requested, as then it does the right thing today while leaving us the option to implement the correct linker semantics later.

llvm-link and nvlink-wrapper both work on whole archive semantics..

> Side point: really not a fan of incorrectly implementing bits of a linker as standalone tools called from clang. Can we raise a bug report against nvlink instead of doing this?

I also don't like adding a new tool just for one additional wrapping around. But, couldn't find a feasible solution on my own. Another alternative is to extend llvm-ar to support "output directory" option. But, the suggestion in last week's multi-company meeting was to go with nvlink-wrapper instead of extending llvm-ar. Let's file a bug report for nvlink for supporting archive files and as soon as it is available we can drop this tool. Requires only one line change in Cuda.cpp.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108291