Skip to content

[SYCL] Link device lib via mlink-builtin-bitcode #2833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from
Closed

[SYCL] Link device lib via mlink-builtin-bitcode #2833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9203c16
Use mlink-builtin-bitcode to link sycl devicelibs
jinge90 Nov 26, 2020
f12fcc8
convert device libs to LLVM BC
jinge90 Nov 30, 2020
b0a57d9
Fix clang format issue
jinge90 Nov 30, 2020
7dfcd9b
Merge remote-tracking branch 'origin/sycl' into LinkDeviceLibViaMlink
jinge90 Dec 1, 2020
2820a54
Fix lit test failure
jinge90 Dec 2, 2020
a35765a
Fix Clang lit test on Windows
jinge90 Dec 2, 2020
37697e5
Using foreach to remove duplicate lines in SYCLDeviceLib CMake
jinge90 Dec 4, 2020
5a9bdf1
Fix merge conflicts
jinge90 Dec 4, 2020
a82798e
Enable libm-fp64 link by default
jinge90 Dec 4, 2020
3afa020
Rename wrapper.h to devicelib_assert.h
jinge90 Dec 7, 2020
faadbe7
Merge remote-tracking branch 'origin/sycl' into LinkDeviceLibViaMlink
jinge90 Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 1 addition & 105 deletions clang/lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3772,16 +3772,6 @@ class OffloadingActionBuilder final {
for (auto SDA : SYCLDeviceActions)
SYCLLinkBinaryList.push_back(SDA);
if (WrapDeviceOnlyBinary) {
// If used without -fintelfpga, -fsycl-link is used to wrap device
// objects for future host link. Device libraries should be linked
// by default to resolve any undefined reference.
if (!Args.hasArg(options::OPT_fintelfpga)) {
const auto *TC = ToolChains.front();
addSYCLDeviceLibs(TC, SYCLLinkBinaryList, true,
C.getDefaultToolChain()
.getTriple()
.isWindowsMSVCEnvironment());
}
// -fsycl-link behavior does the following to the unbundled device
// binaries:
// 1) Link them together using llvm-link
Expand Down Expand Up @@ -3948,92 +3938,6 @@ class OffloadingActionBuilder final {
SYCLDeviceActions.clear();
}

void addSYCLDeviceLibs(const ToolChain *TC, ActionList &DeviceLinkObjects,
bool isSpirvAOT, bool isMSVCEnv) {
enum SYCLDeviceLibType {
sycl_devicelib_wrapper,
sycl_devicelib_fallback
};
struct DeviceLibOptInfo {
StringRef devicelib_name;
StringRef devicelib_option;
};

bool NoDeviceLibs = false;
// Currently, libc, libm-fp32 will be linked in by default. In order
// to use libm-fp64, -fsycl-device-lib=libm-fp64/all should be used.
llvm::StringMap<bool> devicelib_link_info = {
{"libc", true}, {"libm-fp32", true}, {"libm-fp64", false}};
if (Arg *A = Args.getLastArg(options::OPT_fsycl_device_lib_EQ,
options::OPT_fno_sycl_device_lib_EQ)) {
if (A->getValues().size() == 0)
C.getDriver().Diag(diag::warn_drv_empty_joined_argument)
<< A->getAsString(Args);
else {
if (A->getOption().matches(options::OPT_fno_sycl_device_lib_EQ))
NoDeviceLibs = true;

for (StringRef Val : A->getValues()) {
if (Val == "all") {
for (auto &K : devicelib_link_info.keys())
devicelib_link_info[K] = true && !NoDeviceLibs;
break;
}
auto LinkInfoIter = devicelib_link_info.find(Val);
if (LinkInfoIter == devicelib_link_info.end()) {
C.getDriver().Diag(diag::err_drv_unsupported_option_argument)
<< A->getOption().getName() << Val;
}
devicelib_link_info[Val] = true && !NoDeviceLibs;
}
}
}

SmallString<128> LibLoc(TC->getDriver().Dir);
llvm::sys::path::append(LibLoc, "/../lib");
StringRef LibSuffix = isMSVCEnv ? ".obj" : ".o";
SmallVector<DeviceLibOptInfo, 5> sycl_device_wrapper_libs = {
{"libsycl-crt", "libc"},
{"libsycl-complex", "libm-fp32"},
{"libsycl-complex-fp64", "libm-fp64"},
{"libsycl-cmath", "libm-fp32"},
{"libsycl-cmath-fp64", "libm-fp64"}};
// For AOT compilation, we need to link sycl_device_fallback_libs as
// default too.
SmallVector<DeviceLibOptInfo, 5> sycl_device_fallback_libs = {
{"libsycl-fallback-cassert", "libc"},
{"libsycl-fallback-complex", "libm-fp32"},
{"libsycl-fallback-complex-fp64", "libm-fp64"},
{"libsycl-fallback-cmath", "libm-fp32"},
{"libsycl-fallback-cmath-fp64", "libm-fp64"}};
auto addInputs = [&](SYCLDeviceLibType t) {
auto sycl_libs = (t == sycl_devicelib_wrapper)
? sycl_device_wrapper_libs
: sycl_device_fallback_libs;
for (const DeviceLibOptInfo &Lib : sycl_libs) {
if (!devicelib_link_info[Lib.devicelib_option])
continue;
SmallString<128> LibName(LibLoc);
llvm::sys::path::append(LibName, Lib.devicelib_name);
llvm::sys::path::replace_extension(LibName, LibSuffix);
if (llvm::sys::fs::exists(LibName)) {
Arg *InputArg = MakeInputArg(Args, C.getDriver().getOpts(),
Args.MakeArgString(LibName));
auto *SYCLDeviceLibsInputAction =
C.MakeAction<InputAction>(*InputArg, types::TY_Object);
auto *SYCLDeviceLibsUnbundleAction =
C.MakeAction<OffloadUnbundlingJobAction>(
SYCLDeviceLibsInputAction);
addDeviceDepences(SYCLDeviceLibsUnbundleAction);
DeviceLinkObjects.push_back(SYCLDeviceLibsUnbundleAction);
}
}
};
addInputs(sycl_devicelib_wrapper);
if (isSpirvAOT)
addInputs(sycl_devicelib_fallback);
}

void appendLinkDependences(OffloadAction::DeviceDependences &DA) override {
assert(ToolChains.size() == DeviceLinkerInputs.size() &&
"Toolchains and linker inputs sizes do not match.");
Expand Down Expand Up @@ -4125,15 +4029,7 @@ class OffloadingActionBuilder final {
else
LinkObjects.push_back(Input);
}
// FIXME: Link all wrapper and fallback device libraries as default,
// When spv online link is supported by all backends, the fallback
// device libraries are only needed when current toolchain is using
// AOT compilation.
if (!isNVPTX) {
addSYCLDeviceLibs(
*TC, LinkObjects, true,
C.getDefaultToolChain().getTriple().isWindowsMSVCEnvironment());
}

// The linkage actions subgraph leading to the offload wrapper.
// [cond] Means incoming/outgoing dependence is created only when cond
// is true. A function of:
Expand Down
87 changes: 86 additions & 1 deletion clang/lib/Driver/ToolChains/SYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "SYCL.h"
#include "CommonArgs.h"
#include "InputInfo.h"
Expand Down Expand Up @@ -417,10 +416,96 @@ SYCLToolChain::SYCLToolChain(const Driver &D, const llvm::Triple &Triple,
getProgramPaths().push_back(getDriver().Dir);
}

static bool doLLVMBCEmit(llvm::opt::ArgStringList &CC1Args) {
for (auto StrArg : CC1Args)
if (std::string(StrArg) == "-emit-llvm-bc")
return true;
return false;
}

void SYCLToolChain::AddSYCLDeviceLibs(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CC1Args) const {
enum SYCLDeviceLibType { sycl_devicelib_wrapper, sycl_devicelib_fallback };
struct DeviceLibOptInfo {
StringRef devicelib_name;
StringRef devicelib_option;
};

bool NoDeviceLibs = false;
// Currently, libc, libm-fp32 will be linked in by default. In order
// to use libm-fp64, -fsycl-device-lib=libm-fp64/all should be used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see any problems with linking libm-fp64 by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @bader
I think it should be OK to link libm-fp64 by default too. I will find a some machine without fp64 extension to see if any problem exposed.
Thanks very much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no problems. If math functions with doubles are not used there will be no unsupported functions in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bader @mdtoguchi @vzakhari
Have updated the patch to link libm-fp64 by default. But I suggest to re-discuss the behavior of "-fsycl-device-lib=..."
Currently, "-fsycl-device-lib=.." won't impact our "default" link behavior, it seems that current behavior of "-fsycl-device-lib=" is useless after we link all device libraries by default.
Maybe aligning behaviors of "-fsycl-device-lib" and "-fno-sycl-device-lib" is more reasonable, if user addes "-fsycl-device-lib=libm-fp32", we won't do "default" link, instead, we only link libm-fp32 device libraries, the same to libc, libm-fp64...
And I think we should re-consider the "-fsycl-device-lib=..." no matter we choose this PR or #2783 in the end.
Thanks very much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say we have just one option - -fsycl analog for -nostdlib i.e. -fno-sycl-device-lib.
I don't think we really need a fine grained control over which part of standard library is linked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @bader. We shouldn't modify what we link by default with an option that adjusts the adding of libs. Similar to adding -lgcc on the command line, that doesn't override the default libs already linked in. A single option that disables all is reasonable.

llvm::StringMap<bool> devicelib_link_info = {
{"libc", true}, {"libm-fp32", true}, {"libm-fp64", true}};
if (Arg *A = Args.getLastArg(options::OPT_fsycl_device_lib_EQ,
options::OPT_fno_sycl_device_lib_EQ)) {
if (A->getValues().size() == 0)
getDriver().Diag(diag::warn_drv_empty_joined_argument)
<< A->getAsString(Args);
else {
if (A->getOption().matches(options::OPT_fno_sycl_device_lib_EQ))
NoDeviceLibs = true;

for (StringRef Val : A->getValues()) {
if (Val == "all") {
for (auto &K : devicelib_link_info.keys())
devicelib_link_info[K] = true && !NoDeviceLibs;
break;
}
auto LinkInfoIter = devicelib_link_info.find(Val);
if (LinkInfoIter == devicelib_link_info.end()) {
getDriver().Diag(diag::err_drv_unsupported_option_argument)
<< A->getOption().getName() << Val;
}
devicelib_link_info[Val] = true && !NoDeviceLibs;
}
}
}
bool isSpirvAOT = true;
SmallString<128> LibLoc(getDriver().Dir);
llvm::sys::path::append(LibLoc, "/../lib");
SmallVector<DeviceLibOptInfo, 5> sycl_device_wrapper_libs = {
{"libsycl-crt", "libc"},
{"libsycl-complex", "libm-fp32"},
{"libsycl-complex-fp64", "libm-fp64"},
{"libsycl-cmath", "libm-fp32"},
{"libsycl-cmath-fp64", "libm-fp64"}};
// For AOT compilation, we need to link sycl_device_fallback_libs as
// default too.
SmallVector<DeviceLibOptInfo, 5> sycl_device_fallback_libs = {
{"libsycl-fallback-cassert", "libc"},
{"libsycl-fallback-complex", "libm-fp32"},
{"libsycl-fallback-complex-fp64", "libm-fp64"},
{"libsycl-fallback-cmath", "libm-fp32"},
{"libsycl-fallback-cmath-fp64", "libm-fp64"}};

auto addInputs = [&](SYCLDeviceLibType t, StringRef SYCLArchName) {
auto sycl_libs = (t == sycl_devicelib_wrapper) ? sycl_device_wrapper_libs
: sycl_device_fallback_libs;
for (const DeviceLibOptInfo &Lib : sycl_libs) {
if (!devicelib_link_info[Lib.devicelib_option])
continue;
SmallString<128> LibName(LibLoc);
llvm::sys::path::append(LibName, Lib.devicelib_name);
LibName.append("-");
LibName.append(SYCLArchName);
llvm::sys::path::replace_extension(LibName, ".bc");
if (llvm::sys::fs::exists(LibName)) {
CC1Args.push_back("-mlink-builtin-bitcode");
CC1Args.push_back(Args.MakeArgString(LibName.c_str()));
}
}
};
addInputs(sycl_devicelib_wrapper, getTriple().getArchName());
if (isSpirvAOT)
addInputs(sycl_devicelib_fallback, getTriple().getArchName());
}

void SYCLToolChain::addClangTargetOptions(
const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadingKind) const {
if (DeviceOffloadingKind == Action::OFK_SYCL && doLLVMBCEmit(CC1Args))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to do doLLVMBCEmit(CC1Args) check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @bader
If we don't do this check, "-mlink-builtin-bitcode ..." will be added when we generate sycl helper header file. We only need to link those device libraries when generating device code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to link them always. Integration header is generated from Sema, so adding -m* options should have no impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @bader
Since the "-mlink" options have no impact to integration header generation, why don't remove the unused options to make the commands more clean?
Thanks very much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right place to make this decision. We must always link devicelib unless user explicitly disable the linking. We should have some "optimizing" logic here as it's a error prone. I can image a few cases already where this can lead to bugs.

AddSYCLDeviceLibs(DriverArgs, CC1Args);
HostTC.addClangTargetOptions(DriverArgs, CC1Args, DeviceOffloadingKind);
}

Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Driver/ToolChains/SYCL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ class LLVM_LIBRARY_VISIBILITY SYCLToolChain : public ToolChain {
void TranslateTargetOpt(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs, llvm::opt::OptSpecifier Opt,
llvm::opt::OptSpecifier Opt_EQ) const;
void AddSYCLDeviceLibs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const;
};

} // end namespace toolchains
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Driver/sycl-intelfpga-static-lib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// make dummy archive
// Build a fat static lib that will be used for all tests
// RUN: echo "void foo(void) {}" > %t1.cpp
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fintelfpga -fsycl %t1.cpp -c -o %t1_bundle.o
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fintelfpga -fsycl -fno-sycl-device-lib=all %t1.cpp -c -o %t1_bundle.o
// RUN: llvm-ar cr %t.a %t1_bundle.o

/// Check phases with static lib
Expand Down
6 changes: 3 additions & 3 deletions clang/test/Driver/sycl-offload-static-lib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
/// test behaviors of passing a fat static lib
// Build a fat static lib that will be used for all tests
// RUN: echo "void foo(void) {}" > %t1.cpp
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fsycl %t1.cpp -c -o %t1_bundle.o
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fsycl -fno-sycl-device-lib=all %t1.cpp -c -o %t1_bundle.o
// RUN: llvm-ar cr %t.a %t1_bundle.o
// RUN: llvm-ar cr %t_2.a %t1_bundle.o

Expand Down Expand Up @@ -76,7 +76,7 @@
/// ###########################################################################

// RUN: touch %t.a
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fsycl -foffload-static-lib=%t.a -### %s 2>&1 \
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fsycl -fno-sycl-device-lib=all -foffload-static-lib=%t.a -### %s 2>&1 \
// RUN: | FileCheck %s -check-prefix=FOFFLOAD_STATIC_LIB_SRC2
// FOFFLOAD_STATIC_LIB_SRC2: clang{{.*}} "-emit-obj" {{.*}} "-o" "[[HOSTOBJ:.+\.o]]"
// FOFFLOAD_STATIC_LIB_SRC2: ld{{(.exe)?}}" "-r" "-o" {{.*}} "[[HOSTOBJ]]" "[[INPUT:.+\.a]]"
Expand All @@ -86,7 +86,7 @@
/// ###########################################################################

// RUN: touch %t.a
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fsycl -foffload-static-lib=%t.a -o output_name -lOpenCL -### %s 2>&1 \
// RUN: %clangxx -target x86_64-unknown-linux-gnu -fsycl -fno-sycl-device-lib=all -foffload-static-lib=%t.a -o output_name -lOpenCL -### %s 2>&1 \
// RUN: | FileCheck %s -check-prefix=FOFFLOAD_STATIC_LIB_SRC3
// FOFFLOAD_STATIC_LIB_SRC3: ld{{(.exe)?}}" "-r" "-o" {{.*}} "[[INPUT:.+\.a]]"
// FOFFLOAD_STATIC_LIB_SRC3: clang-offload-bundler{{.*}} "-type=oo"
Expand Down
Loading