[CUDA][HIP] Disable emitting llvm.linker.options in device compilation

[yaxunl]

Summary

The linker options (e.g. pragma detect_mismatch) are intended for host compilation only, therefore disable it for device compilation.

[tra]

Could you elaborate on why you want to disable this metadata? I think the original idea of llvm.linker.options was that it should be ignored if the back-end does not support it.

lib/CodeGen/CodeGenModule.cpp

441 ↗

(On Diff #185589)

If we do need to disable it, it may be better to do it in
ShouldDisableAutolink() function in clang/lib/Driver/ToolChains/Clang.cpp

[yaxunl]

In D57829#1387412, @tra wrote:

Could you elaborate on why you want to disable this metadata? I think the original idea of llvm.linker.options was that it should be ignored if the back-end does not support it.

If backend does not support it, it goes to TargetLoweringObjectFileELF::emitModuleMetadata and causes codegen to fail.

[tra]

In D57829#1387416, @yaxunl wrote:

In D57829#1387412, @tra wrote:

Could you elaborate on why you want to disable this metadata? I think the original idea of llvm.linker.options was that it should be ignored if the back-end does not support it.

If backend does not support it, it goes to TargetLoweringObjectFileELF::emitModuleMetadata and causes codegen to fail.

Fails how? AFAICT, for llvm.linker.options emitModuleMetadata() just creates an ELF section ".linker-options" which should just work.
Are you saying that some of HIP tools can't deal with the unknown section type?

[rjmccall]

Also, why is this HIP-specific? I thought the toolchain was largely shared with CUDA and there were just a few runtime differences.

[yaxunl]

no longer needed

[yaxunl]

we still need this

[yaxunl]

use -fno-autolink to disable linker option for device compilation.

[tra]

Could you, please, give us a bit more context and provide more info for the questions @rjmccall and I asked before?

Is the problem specifically because autolink is not supported on device side? Or is disabling autolink just a convoluted way to avoid calling EmitModuleLinkOptions()?
If it's the former, then we should just disable it unconditionally -- we already filter out some other flags (e.g. asan).
If it's the latter, then tweaking autolink option behavior is just covering the problem. Sooner or later EmitModuleLinkOptions() will be called by something else and we'll be back to where we are now.

[yaxunl]

In D57829#1729094, @tra wrote:

Could you, please, give us a bit more context and provide more info for the questions @rjmccall and I asked before?

Is the problem specifically because autolink is not supported on device side? Or is disabling autolink just a convoluted way to avoid calling EmitModuleLinkOptions()?
If it's the former, then we should just disable it unconditionally -- we already filter out some other flags (e.g. asan).
If it's the latter, then tweaking autolink option behavior is just covering the problem. Sooner or later EmitModuleLinkOptions() will be called by something else and we'll be back to where we are now.

we need to disable autolink for device compilation because the linker options are intended for host compilation.

Previously I said the backend does not support linker option, which was incorrect. The diagnostic about invalid linker option was due to a clang codegen bug due to detect_mismatch, which I have a fix

https://reviews.llvm.org/D69678

Even with that fix, we still need to disable auto link for device compilation, since it is intended for host compilation.

[tra]

clang/lib/Driver/ToolChains/Clang.cpp

389–392

Shouldn't it be true, considering that we do want to disable autolinking by default for device-side CUDA/HIP?

If we don't support autolinking at all for CUDA/HIP, perhaps we should just return true here.

[yaxunl]

clang/lib/Driver/ToolChains/Clang.cpp

389–392

This variable Default is to be used as the default value of OPT_fautolink. For device compilation we want the default value to be false. However if users want to force it to be true, we may still want to respect it.

[tra]

clang/lib/Driver/ToolChains/Clang.cpp

389–392

You are correct. I've missed the ! in the return below. Don't never use double negation. :-/

Nit: Perhaps we can rename Default -> AutolinkEnabledByDefault because right now it's easy to misinterpret it as the default return value of the function. Maybe, even change the function itself to ShouldEnableAutolink().

[yaxunl]

clang/lib/Driver/ToolChains/Clang.cpp

389–392

will change it to ShouldEnableAutolink

[yaxunl]

revised by Artem's comments. Also fix test since linker option only supported on windows.

[tra]

LGTM in general. Nit about test name and test scope.

clang/test/CodeGenCUDA/linker-options.cu

1–5 ↗

(On Diff #227636)

This appears to be specific to HIP on windows. If that's intended, then the file should be renamed to something like hip-ms-linker-options.cu. If the changes in functionality apply on other platforms, or with CUDA, the file name can remain as is, but it would be great to add some test runs for the other uses cases.

[yaxunl]

clang/test/CodeGenCUDA/linker-options.cu

1–5 ↗

(On Diff #227636)

the change in functionality applies to HIP and CUDA on windows only. will rename it to ms-linker-options.cu and add test for CUDA when committing.

clang/lib/Driver/ToolChains/Clang.cpp

if (CXXExceptionsEnabled) {
       EH = true;
     }
   }
 
   if (EH)
     CmdArgs.push_back("-fexceptions");
 }
 
-static bool ShouldDisableAutolink(const ArgList &Args, const ToolChain &TC) {
+static bool ShouldEnableAutolink(const ArgList &Args, const ToolChain &TC,
+                                 const JobAction &JA) {
   bool Default = true;
   if (TC.getTriple().isOSDarwin()) {
     // The native darwin assembler doesn't support the linker_option directives,
     // so we disable them if we think the .s file will be passed to it.
     Default = TC.useIntegratedAs();
   }
-  return !Args.hasFlag(options::OPT_fautolink, options::OPT_fno_autolink,
-                       Default);
+  // The linker_option directives are intended for host compilation.
+  if (JA.isDeviceOffloading(Action::OFK_Cuda) ||
+      JA.isDeviceOffloading(Action::OFK_HIP))
+    Default = false;
+  return Args.hasFlag(options::OPT_fautolink, options::OPT_fno_autolink,
 }
 
 static bool ShouldDisableDwarfDirectory(const ArgList &Args,
                                         const ToolChain &TC) {
   bool UseDwarfDirectory =
       Args.hasFlag(options::OPT_fdwarf_directory_asm,
                    options::OPT_fno_dwarf_directory_asm, TC.useIntegratedAs());
   return !UseDwarfDirectory;
if (Asm->getOption().matches(options::OPT_fasm))
       CmdArgs.push_back("-fgnu-keywords");
     else
       CmdArgs.push_back("-fno-gnu-keywords");
   }
 
   if (ShouldDisableDwarfDirectory(Args, TC))
     CmdArgs.push_back("-fno-dwarf-directory-asm");
 
-  if (ShouldDisableAutolink(Args, TC))
+  if (!ShouldEnableAutolink(Args, TC, JA))
     CmdArgs.push_back("-fno-autolink");
 
   // Add in -fdebug-compilation-dir if necessary.
   addDebugCompDirArg(Args, CmdArgs, D.getVFS());
 
   addDebugPrefixMapArg(D, Args, CmdArgs);
 
   if (Arg *A = Args.getLastArg(options::OPT_ftemplate_depth_,

clang/test/CodeGenCUDA/ms-linker-options.cu

+// RUN: %clang_cc1 -emit-llvm -o - -fcuda-is-device -fms-extensions -x hip %s \
+// RUN:   -fno-autolink -triple amdgcn-amd-amdhsa \
+// RUN:   | FileCheck -check-prefix=DEV %s
+// RUN: %clang_cc1 -emit-llvm -o - -fms-extensions -x hip %s -triple \
+// RUN:    x86_64-pc-windows-msvc | FileCheck -check-prefix=HOST %s
+// RUN: %clang_cc1 -emit-llvm -o - -fcuda-is-device -fms-extensions %s \
+// RUN:   -fno-autolink -triple amdgcn-amd-amdhsa \
+// RUN:   | FileCheck -check-prefix=DEV %s
+// RUN: %clang_cc1 -emit-llvm -o - -fms-extensions %s -triple \
+// RUN:    x86_64-pc-windows-msvc | FileCheck -check-prefix=HOST %s
+
+// DEV-NOT: llvm.linker.options
+// DEV-NOT: llvm.dependent-libraries
+// HOST: lvm.linker.options
+// HOST: "/DEFAULTLIB:libcpmt.lib"
+// HOST: "/FAILIFMISMATCH:\22myLib_version=9\22"
+
+#pragma comment(lib, "libcpmt")
+#pragma detect_mismatch("myLib_version", "9")

clang/test/Driver/hip-autolink.hip

+// REQUIRES: clang-driver
+// REQUIRES: x86-registered-target
+// REQUIRES: amdgpu-registered-target
+//
+// RUN: %clang --target=i386-pc-windows-msvc --cuda-gpu-arch=gfx906 -nogpulib \
+// RUN:   --cuda-device-only -x hip %s -### 2>&1 | FileCheck --check-prefix=DEV %s
+// RUN: %clang --target=i386-pc-windows-msvc --cuda-gpu-arch=gfx906 -nogpulib \
+// RUN:   --cuda-host-only -x hip %s -### 2>&1 | FileCheck --check-prefix=HOST %s
+
+// DEV: "-cc1" "-triple" "amdgcn-amd-amdhsa"
+// DEV-SAME: "-fno-autolink"
+
+// HOST: "-cc1" "-triple" "i386-pc-windows-msvc{{.*}}"
+// HOST-NOT: "-fno-autolink"