[OpenMP] Pass AMDGPU math libraries into the linker wrapper

[jhuber6]

Summary

This patch passes in the AMDPGU math libraries to the linker wrapper.
The wrapper already handles linking OpenMP bitcode libraries via the
--target-library option. This should be sufficient to link in math
libraries for the accompanying architecture.

Fixes #53526.

[jhuber6]

I can't test this yet because the AMDGPU cluster is down for maintenance.

[jdoerfert]

clang/lib/Driver/ToolChains/Clang.cpp

8203

Can you elaborate on this comment, what's bad, how would the better version look

[jhuber6]

clang/lib/Driver/ToolChains/Clang.cpp

8203

It's explained in more detail where this is done for the AMDGPU ToolChain, e.g.

// This is not certain to work. The device libs added here, and passed to    
// llvm-link, are missing attributes that they expect to be inserted when    
// passed to mlink-builtin-bitcode. The amdgpu backend does not generate    
// conservatively correct code when attributes are missing, so this may    
// be the root cause of miscompilations. Passing via mlink-builtin-bitcode    
// ultimately hits CodeGenModule::addDefaultFunctionDefinitionAttributes    
// on each function, see D28538 for context.    
// Potential workarounds:    
//  - unconditionally link all of the device libs to every translation    
//    unit in clang via mlink-builtin-bitcode    
//  - build a libm bitcode file as part of the DeviceRTL and explictly    
//    mlink-builtin-bitcode the rocm device libs components at build time    
//  - drop this llvm-link fork in favour or some calls into LLVM, chosen    
//    to do basically the same work as llvm-link but with that call first    
//  - write an opt pass that sets that on every function it sees and pipe    
//    the device-libs bitcode through that on the way to this llvm-link

Should I copy the gist here?

[jdoerfert]

clang/lib/Driver/ToolChains/Clang.cpp

8203

Is it still relevant? We don't use llvm-link here, do we?

@arsenm, the backend is (almost) OK with the lack of attributes, is it not?

8213

I'd switch the conditions.

More importantly, does this require that the user passes -lm to the linker invocation? I'm not convinced we should not always link these in.

[jhuber6]

clang/lib/Driver/ToolChains/Clang.cpp

8203

Linking is done using LTO now, I don't know exactly how they merge bitcode compared to llvm-link but I'm assuming it's similar.

8213

Yes, would save some time assuming most codes are C++

So I figured I'd copy the same semantics of how -lm works where you need to specify it for C but not C++. We could just pass this in all the time, but since linking it in currently required -lm I copied that.

[JonChesterfield]

I'm not confident it's only attributes that we rely on mlink-builtin-bitcode patching up, that could be poor phrasing on my part.

Making the pipeline robust to underspecified IR input may be easier (and arguably more useful) than changing the rocm library to be compiled for a given target.

clang/lib/Driver/ToolChains/Clang.cpp

8213

re: always linking these libraries in, regardless of -lm, it's probably better to link them by default and effectively ignore -lm.

I'd like to keep it guarded by -nogpulib or similar so that people can still opt out of the rocm device library stack entirely.

[jdoerfert]

clang/lib/Driver/ToolChains/Clang.cpp

8213

Opting out of "optional" gpu libraries is fine. I want to avoid that one needs to add linking flags etc. for OpenMP. We link libdevice by default too, so this is no different (as both contain math function impl etc.)

[jhuber6]

Removing -lm semantics so it is linked by default. Also adding the flag to the
tested output.

[JonChesterfield]

clang/lib/Driver/ToolChains/Clang.cpp

8213

I guess that hangs on whether libm is optional or not.

I am reluctant to tie every openmp execution to the rocm libdevice as it frequently fails to build with llvm trunk and I'm nervy about linking IR built by the rocm clang with IR generated from trunk. It mostly seems to work for rocm sufficiently similar in age to trunk but periodically the abi or intrinsic set diverges.

Maybe our working assumption should be that everyone using openmp wants libm so rocm breaking trunk openmp has to be resolved each time it happens anyway. If so, there's a design decision to be made about how to manage that dependency.

[jhuber6]

clang/lib/Driver/ToolChains/Clang.cpp

8213

I tested this on Spock and it resolves linking math libraries. I'm not sure about overall stability however. I'm assuming this is sufficient until we get a real libdevice for AMDGPU.

[arsenm]

clang/lib/Driver/ToolChains/Clang.cpp

8203

It can be subtley wrong, and also pushes reliance on setting the global subtarget. It's best to ensure the default attributes are propagated to the device libraries

[jdoerfert]

LG, with or without the -lm lookup

clang/lib/Driver/ToolChains/Clang.cpp

8203

We will build a better solution for this soon, for now this is "good enough".

[ronlieb]

Hi Joseph,
seems to have broken amdgpu buildbit,
please revert and fix ?
https://lab.llvm.org/buildbot/#/builders/193/builds/6939

[JonChesterfield]

This is asserting somewhere in clang-linker-wrapper, e.g. https://lab.llvm.org/buildbot/#/builders/193/builds/6939

[jhuber6]

In D119841#3326638, @JonChesterfield wrote:

This is asserting somewhere in clang-linker-wrapper, e.g. https://lab.llvm.org/buildbot/#/builders/193/builds/6939

Yes, must be when we try to sort the device files. I'll revert for now since I don't have time to fix it until later.

[JonChesterfield]

Thanks!

clang/lib/Driver/ToolChains/Clang.cpp

if (TC->getTriple().isNVPTX()) {
       CudaInstallationDetector CudaInstallation(D, TheTriple, Args);
       if (CudaInstallation.isValid())
         CmdArgs.push_back(Args.MakeArgString(
             "--cuda-path=" + CudaInstallation.getInstallPath()));
       break;
     }
   }
 
+  // Get the AMDGPU math libraries.
+  // FIXME: This method is bad, remove once AMDGPU has a proper math library
+  // (see AMDGCN::OpenMPLinker::constructLLVMLinkCommand).
+  for (auto &I : llvm::make_range(OpenMPTCRange.first, OpenMPTCRange.second)) {
+    const ToolChain *TC = I.second;
+
+    if (!TC->getTriple().isAMDGPU() || Args.hasArg(options::OPT_nogpulib))
+      continue;
+
+    const ArgList &TCArgs = C.getArgsForToolChain(TC, "", Action::OFK_OpenMP);
+    StringRef Arch = TCArgs.getLastArgValue(options::OPT_march_EQ);
+    const toolchains::ROCMToolChain RocmTC(TC->getDriver(), TC->getTriple(),
+                                           TCArgs);
+
+    SmallVector<std::string, 12> BCLibs =
+        RocmTC.getCommonDeviceLibNames(TCArgs, Arch.str());
+
+    for (StringRef LibName : BCLibs)
+      CmdArgs.push_back(
+          Args.MakeArgString("-target-library=" + TC->getTripleString() + "-" +
+                             Arch + "=" + LibName));
+  }
+
   if (D.isUsingLTO(/* IsOffload */ true)) {
     // Pass in target features for each toolchain.
     for (auto &I :
          llvm::make_range(OpenMPTCRange.first, OpenMPTCRange.second)) {
       const ToolChain *TC = I.second;
       const ArgList &TCArgs = C.getArgsForToolChain(TC, "", Action::OFK_OpenMP);
       ArgStringList FeatureArgs;
       TC->addClangTargetOptions(TCArgs, FeatureArgs, Action::OFK_OpenMP);

clang/test/Driver/amdgpu-openmp-toolchain.c

 // CHECK-C: "x86_64-unknown-linux-gnu" - "clang::as"
 // CHECK-C: "x86_64-unknown-linux-gnu" - "offload bundler"
 
 // RUN:   %clang -### --target=x86_64-unknown-linux-gnu -emit-llvm -S -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx803 -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-EMIT-LLVM-IR
 // CHECK-EMIT-LLVM-IR: clang{{.*}}"-cc1"{{.*}}"-triple" "amdgcn-amd-amdhsa"{{.*}}"-emit-llvm"
 
 // RUN: %clang -### -target x86_64-pc-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx803 -lm --rocm-device-lib-path=%S/Inputs/rocm/amdgcn/bitcode %s 2>&1 | FileCheck %s --check-prefix=CHECK-LIB-DEVICE
 // CHECK-LIB-DEVICE: {{.*}}llvm-link{{.*}}ocml.bc"{{.*}}ockl.bc"{{.*}}oclc_daz_opt_on.bc"{{.*}}oclc_unsafe_math_off.bc"{{.*}}oclc_finite_only_off.bc"{{.*}}oclc_correctly_rounded_sqrt_on.bc"{{.*}}oclc_wavefrontsize64_on.bc"{{.*}}oclc_isa_version_803.bc"
+
+// RUN: %clang -### -target x86_64-pc-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx803 -lm --rocm-device-lib-path=%S/Inputs/rocm/amdgcn/bitcode -fopenmp-new-driver %s 2>&1 | FileCheck %s --check-prefix=CHECK-LIB-DEVICE-NEW
+// CHECK-LIB-DEVICE-NEW: {{.*}}clang-linker-wrapper{{.*}}-target-library=amdgcn-amd-amdhsa-gfx803={{.*}}ocml.bc"{{.*}}ockl.bc"{{.*}}oclc_daz_opt_on.bc"{{.*}}oclc_unsafe_math_off.bc"{{.*}}oclc_finite_only_off.bc"{{.*}}oclc_correctly_rounded_sqrt_on.bc"{{.*}}oclc_wavefrontsize64_on.bc"{{.*}}oclc_isa_version_803.bc"