[Clang] Respect `-L` options when compiling directly for AMDGPU

[jhuber6]

Summary

The AMDGPU linker is lld, which has full support for standard features
like static libraries. Previously the AMDGPU toolchain did not forward
-L arguments so we could not tell it where to find certain libraries.
This patch simply forwards it like the other toolchains.

[yaxunl]

clang/lib/Driver/ToolChains/AMDGPU.cpp

546 ↗

(On Diff #520017)

AddLinkerInputs has code doing that, and it handles env var LIBRARY_PATH. However that code is disabled for AMDGPU because AMDGPU returns true for isCrossCompiling.

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L236

It seems isCrossCompiling is solely for controlling whether to consume -L. If we want amdgpu toolchain to accept -L, we can simply let isCrossCompiling return false.

[jhuber6]

clang/lib/Driver/ToolChains/AMDGPU.cpp

546 ↗

(On Diff #520017)

Good catch, we could maybe set isCrossCompiling to false if targeted directly by the user, e.g. --target=amdgcn-amd-amdhsa vs --offload-arch.

[yaxunl]

clang/lib/Driver/ToolChains/AMDGPU.cpp

546 ↗

(On Diff #520017)

That would be better. Thanks.

[jhuber6]

clang/lib/Driver/ToolChains/AMDGPU.cpp

546 ↗

(On Diff #520017)

It still is technically cross compiling, since we are building for a target that does not match the system's architecture. The original code that prevents passing -L was contributed by @MaskRay. I understand that we may not want to pass LIBRARY_PATH defines, but what's the rationale for not passing any -L options manually specified by the user?

[MaskRay]

clang/lib/Driver/ToolChains/AMDGPU.cpp

546 ↗

(On Diff #520017)

The LIBRARY_PATH code had been there when I moved it in 2019. It'd be best not to rely on LIBRARY_PATH. I think that forwarding -L seems reasonable but I am not familiar with the amdgpu ecosystem..

[jhuber6]

clang/lib/Driver/ToolChains/AMDGPU.cpp

546 ↗

(On Diff #520017)

The linker is just lld so it should be the same conceptually. I'm just figuring that even if the user is cross compiling we should respect -L passed on the command line. Should I change this patch to make that change?

[MaskRay]

The linker is just lld so it should be the same conceptually. I'm just figuring that even if the user is cross compiling we should respect -L passed on the command line. Should I change this patch to make that change?

So it seems that there are configurations that we need -L (cross compilation?). If we forward -L in some configurations, I think it'd be better to do this consistently.

The LIBRARY_PATH options seems not useful and the conditional if (not cross compiling) add LIBRARY_PATH is more unfortunate. I wish that we don't add more cases that we do something with LIBRARY_PATH.
(IMO the LIBRARY_PATH users can just fix their build system or use CCC_OVERRIDE_OPTIONS)

We definitely want to avoid more uses of TC.isCrossCompiling(). Most parts of the driver are written in a way that is TC.isCrossCompiling() agnostic.

[jhuber6]

In D150013#4330452, @MaskRay wrote:

So it seems that there are configurations that we need -L (cross compilation?). If we forward -L in some configurations, I think it'd be better to do this consistently.

The LIBRARY_PATH options seems not useful and the conditional if (not cross compiling) add LIBRARY_PATH is more unfortunate. I wish that we don't add more cases that we do something with LIBRARY_PATH.
(IMO the LIBRARY_PATH users can just fix their build system or use CCC_OVERRIDE_OPTIONS)

We definitely want to avoid more uses of TC.isCrossCompiling().

The comment itself // LIBRARY_PATH are included before user inputs and only supported on native toolchains. suggests that something like this might be valid.

// LIBRARY_PATH are included before user inputs and only supported on native
// toolchains.
if (!TC.isCrossCompiling())
  addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH");
else 
  Args.AddAllArgs(CmdArgs, options::OPT_L);

clang/lib/Driver/ToolChains/AMDGPU.cpp

546 ↗

(On Diff #520017)

For offloading I think the problem might be that we don't want to forward -L arguments from the host. In that case, we might be able to apply `

[jhuber6]

Updating, @yaxunl does this look good?

[yaxunl]

LGTM. Thanks.

[MaskRay]

Sorry, but I just realized that updating CommonArgs.cpp is wrong... I have fixed it with commit 681cb54a54bb60dea9034b4b0b45ccc392875b9a.

clang/lib/Driver/ToolChains/CommonArgs.cpp

void tools::AddLinkerInputs(const ToolChain &TC, const InputInfoList &Inputs,
                             const JobAction &JA) {
   const Driver &D = TC.getDriver();
 
   // Add extra linker input arguments which are not treated as inputs
   // (constructed via -Xarch_).
   Args.AddAllArgValues(CmdArgs, options::OPT_Zlinker_input);
 
   // LIBRARY_PATH are included before user inputs and only supported on native
-  // toolchains.
+  // toolchains. Otherwise only add the '-L' arguments requested by the user.
   if (!TC.isCrossCompiling())
     addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH");
+  else
+    Args.AddAllArgs(CmdArgs, options::OPT_L);
 
   for (const auto &II : Inputs) {
     // If the current tool chain refers to an OpenMP offloading host, we
     // should ignore inputs that refer to OpenMP offloading devices -
     // they will be embedded according to a proper linker script.
     if (auto *IA = II.getAction())
       if ((JA.isHostOffloading(Action::OFK_OpenMP) &&
            IA->isDeviceOffloading(Action::OFK_OpenMP)))

clang/test/Driver/amdgpu-toolchain.c

 // RUN: %clang -### --target=amdgcn--amdhsa -x assembler -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=AS_LINK %s
 // RUN: %clang -### -g --target=amdgcn--amdhsa -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=DWARF_VER %s
 // RUN: %clang -### --target=amdgcn-amd-amdpal -x assembler -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=AS_LINK %s
 // RUN: %clang -### -g --target=amdgcn-amd-amdpal -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=DWARF_VER %s
 // RUN: %clang -### --target=amdgcn-mesa-mesa3d -x assembler -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=AS_LINK %s
 // RUN: %clang -### -g --target=amdgcn-mesa-mesa3d -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=DWARF_VER %s
 
 // AS_LINK: "-cc1as"
 // AS_LINK: ld.lld{{.*}} "--no-undefined" "-shared"
 
 // DWARF_VER: "-dwarf-version=5"
 
 // RUN: %clang -### --target=amdgcn-amd-amdhsa -mcpu=gfx906 -nogpulib \
-// RUN:   -flto -fconvergent-functions %s 2>&1 | FileCheck -check-prefix=LTO %s
+// RUN:   -L. -flto -fconvergent-functions %s 2>&1 | FileCheck -check-prefix=LTO %s
 // LTO: clang{{.*}} "-flto=full"{{.*}}"-fconvergent-functions"
-// LTO: ld.lld{{.*}}-plugin-opt=mcpu=gfx906
+// LTO: ld.lld{{.*}}"-L."{{.*}}"-plugin-opt=mcpu=gfx906"