[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

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

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

That would be better. Thanks.

[jhuber6]

clang/lib/Driver/ToolChains/AMDGPU.cpp

546

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

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

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

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/AMDGPU.cpp

void amdgpu::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                   const InputInfo &Output,
                                   const InputInfoList &Inputs,
                                   const ArgList &Args,
                                   const char *LinkingOutput) const {
 
   std::string Linker = getToolChain().GetProgramPath(getShortName());
   ArgStringList CmdArgs;
   addLinkerCompressDebugSectionsOption(getToolChain(), Args, CmdArgs);
   AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
+  Args.AddAllArgs(CmdArgs, options::OPT_L);
   if (C.getDriver().isUsingLTO())
     addLTOOptions(getToolChain(), Args, CmdArgs, Output, Inputs[0],
                   C.getDriver().getLTOMode() == LTOK_Thin);
   CmdArgs.push_back("--no-undefined");
   CmdArgs.push_back("-shared");
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
   C.addCommand(std::make_unique<Command>(

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"