[PATCH] D158582: [AMDGPU] Respect unresolved symbol option if forwarded to linker

[Jacob Lambert via Phabricator via cfe-commits]

lamb-j retitled this revision from "WIP: [AMDGPU] Respect unresolved symbol 
option if forwarded to linker" to "[AMDGPU] Respect unresolved symbol option if 
forwarded to linker".
lamb-j updated this revision to Diff 552775.
lamb-j requested review of this revision.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158582

Files:
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/test/Driver/amdgpu-toolchain-opencl.cl
  clang/test/Driver/amdgpu-toolchain.c


Index: clang/test/Driver/amdgpu-toolchain.c
===
--- clang/test/Driver/amdgpu-toolchain.c
+++ clang/test/Driver/amdgpu-toolchain.c
@@ -10,6 +10,14 @@
 
 // DWARF_VER: "-dwarf-version=5"
 
+// RUN: %clang -### --target=amdgcn--amdhsa -x assembler \
+// RUN:  -Wl,--unresolved-symbols=ignore-all %s 2>&1 | FileCheck 
-check-prefix=AS_LINK_UR %s
+// RUN: %clang -### --target=amdgcn--amdhsa -x assembler \
+// RUN:  -Xlinker --unresolved-symbols=ignore-all %s 2>&1 | FileCheck 
-check-prefix=AS_LINK_UR %s
+
+// AS_LINK_UR: "-cc1as"
+// AS_LINK_UR: ld.lld{{.*}} "--unresolved-symbols=ignore-all"
+
 // RUN: %clang -### --target=amdgcn-amd-amdhsa -mcpu=gfx906 -nogpulib \
 // RUN:   -L. -flto -fconvergent-functions %s 2>&1 | FileCheck 
-check-prefixes=LTO,MCPU %s
 // RUN: %clang -### --target=amdgcn-amd-amdhsa -mcpu=gfx906 -nogpulib \
Index: clang/test/Driver/amdgpu-toolchain-opencl.cl
===
--- clang/test/Driver/amdgpu-toolchain-opencl.cl
+++ clang/test/Driver/amdgpu-toolchain-opencl.cl
@@ -28,3 +28,7 @@
 
 // RUN: %clang -### --target=amdgcn-amd-amdhsa-opencl -x cl -mcpu=fiji 
-nogpulib %s 2>&1 | FileCheck -check-prefix=CHK-LINK %s
 // CHK-LINK: ld.lld{{.*}} "--no-undefined" "-shared"
+
+// RUN: %clang -### --target=amdgcn-amd-amdhsa-opencl 
-Wl,--unresolved-symbols=ignore-all -x cl -mcpu=fiji -nogpulib %s 2>&1 | 
FileCheck -check-prefix=CHK-LINK_UR %s
+// RUN: %clang -### --target=amdgcn-amd-amdhsa-opencl -Xlinker 
--unresolved-symbols=ignore-all -x cl -mcpu=fiji -nogpulib %s 2>&1 | FileCheck 
-check-prefix=CHK-LINK_UR %s
+// CHK-LINK_UR: ld.lld{{.*}} "--unresolved-symbols=ignore-all"
Index: clang/lib/Driver/ToolChains/AMDGPU.cpp
===
--- clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -551,6 +551,9 @@
 
   std::string Linker = getToolChain().GetProgramPath(getShortName());
   ArgStringList CmdArgs;
+  CmdArgs.push_back("--no-undefined");
+  CmdArgs.push_back("-shared");
+
   addLinkerCompressDebugSectionsOption(getToolChain(), Args, CmdArgs);
   Args.AddAllArgs(CmdArgs, options::OPT_L);
   AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
@@ -560,8 +563,6 @@
   else if (Args.hasArg(options::OPT_mcpu_EQ))
 CmdArgs.push_back(Args.MakeArgString(
 "-plugin-opt=mcpu=" + Args.getLastArgValue(options::OPT_mcpu_EQ)));
-  CmdArgs.push_back("--no-undefined");
-  CmdArgs.push_back("-shared");
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
   C.addCommand(std::make_unique(


Index: clang/test/Driver/amdgpu-toolchain.c
===
--- clang/test/Driver/amdgpu-toolchain.c
+++ clang/test/Driver/amdgpu-toolchain.c
@@ -10,6 +10,14 @@
 
 // DWARF_VER: "-dwarf-version=5"
 
+// RUN: %clang -### --target=amdgcn--amdhsa -x assembler \
+// RUN:  -Wl,--unresolved-symbols=ignore-all %s 2>&1 | FileCheck -check-prefix=AS_LINK_UR %s
+// RUN: %clang -### --target=amdgcn--amdhsa -x assembler \
+// RUN:  -Xlinker --unresolved-symbols=ignore-all %s 2>&1 | FileCheck -check-prefix=AS_LINK_UR %s
+
+// AS_LINK_UR: "-cc1as"
+// AS_LINK_UR: ld.lld{{.*}} "--unresolved-symbols=ignore-all"
+
 // RUN: %clang -### --target=amdgcn-amd-amdhsa -mcpu=gfx906 -nogpulib \
 // RUN:   -L. -flto -fconvergent-functions %s 2>&1 | FileCheck -check-prefixes=LTO,MCPU %s
 // RUN: %clang -### --target=amdgcn-amd-amdhsa -mcpu=gfx906 -nogpulib \
Index: clang/test/Driver/amdgpu-toolchain-opencl.cl
===
--- clang/test/Driver/amdgpu-toolchain-opencl.cl
+++ clang/test/Driver/amdgpu-toolchain-opencl.cl
@@ -28,3 +28,7 @@
 
 // RUN: %clang -### --target=amdgcn-amd-amdhsa-opencl -x cl -mcpu=fiji -nogpulib %s 2>&1 | FileCheck -check-prefix=CHK-LINK %s
 // CHK-LINK: ld.lld{{.*}} "--no-undefined" "-shared"
+
+// RUN: %clang -### --target=amdgcn-amd-amdhsa-opencl -Wl,--unresolved-symbols=ignore-all -x cl -mcpu=fiji -nogpulib %s 2>&1 | FileCheck -check-prefix=CHK-LINK_UR %s
+// RUN: %clang -### --target=amdgcn-amd-amdhsa-opencl -Xlinker --unresolved-symbols=ignore-all -x cl -mcpu=fiji -nogpulib %s 2>&1 | FileCheck -check-prefix=CHK-LINK_UR %s
+// CHK-LINK_UR: ld.lld{{.*}} "--unresolved-symbols=ignore-all"
Index: clang/lib/Driver/ToolChains/AMDGPU.cpp
=

[PATCH] D158582: [AMDGPU] Respect unresolved symbol option if forwarded to linker

[Jacob Lambert via Phabricator via cfe-commits]

lamb-j added a comment.

Is void amdgpu::Linker::ConstructJob() constructing a job for the host linker 
or the device linker? Or both?

> The `-Wl` and `-Xlinker` options are intended for the host linker and we 
> intentionally do not pass them to the device linker.

In the example I'm testing, both -Wl and -Xlinker result in a forwarded option 
to the lld invocation. However, when testing with `-Xoffload-linker` , I just 
get the following:

  warning: argument unused during compilation: '-Xoffload-linker 
--unresolved-symbols=ignore-all' [-Wunused-command-line-argument]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158582

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158582: [AMDGPU] Respect unresolved symbol option if forwarded to linker

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

In D158582#4610023 , @yaxunl wrote:

> The `-Wl` and `-Xlinker` options are intended for the host linker and we 
> intentionally do not pass them to the device linker.
>
> If users want to pass options to the device linker, they need to use 
> -Xoffload-linker.
>
> There are multiple options affecting the handling of unresolved symbols. I 
> think the proper way is to use `--no-undefined` as the default, which is 
> always passed before those options from `-Xoffload-linker` so that the latter 
> can override the former.
>
> I believe the driver already passes `-Xoffload-linker`  options to 
> amdgpu::Linker::ConstructJob by Args. I think we probably only need to move 
> all the default options (line 563-566) to 554 so that they can be overridden.

Yeah, the original problem was not being able to overload the defaults. This 
fundamentally is just because the `-Wl` options are being added in 
`AddLinkerInputs` before the defaults. Surprised I didn't catch that, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158582

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158582: [AMDGPU] Respect unresolved symbol option if forwarded to linker

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

The `-Wl` and `-Xlinker` options are intended for the host linker and we 
intentionally do not pass them to the device linker.

If users want to pass options to the device linker, they need to use 
-Xoffload-linker.

There are multiple options affecting the handling of unresolved symbols. I 
think the proper way is to use `--no-undefined` as the default, which is always 
passed before those options from `-Xoffload-linker` so that the latter can 
override the former.

I believe the driver already passes `-Xoffload-linker`  options to 
amdgpu::Linker::ConstructJob by Args. I think we probably only need to move all 
the default options (line 563-566) to 554 so that they can be overridden.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158582

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158582: [AMDGPU] Respect unresolved symbol option if forwarded to linker

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

Remember to clang format.




Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:564-575
+  // If the user has manually passed -Wl,--unresolved-symbols=* as a linker
+  // option, we should not add --no-undefined
+  bool UnresolvedOpt = false;
+  for (auto A : Args)
+if (A->getOption().matches(options::OPT_Wl_COMMA) ||
+ A->getOption().matches(options::OPT_Xlinker))
+  for (StringRef V : A->getValues())

A little more concisely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158582

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits