Skip to content
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

[Driver][clang-linker-wrapper] Add initial support for OpenMP offloading to generic SPIR-V #120145

Merged
merged 14 commits into from
Jan 2, 2025
Merged

[Driver][clang-linker-wrapper] Add initial support for OpenMP offloading to generic SPIR-V #120145

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e1b9b50
[Driver][clang-linker-wrapper] Add initial support for OpenMP offload…
sarnex Dec 16, 2024
5a93fdf
clang format
sarnex Dec 16, 2024
9a0a9f3
fix lit test
sarnex Dec 17, 2024
0557f89
remove spir-v fix
sarnex Dec 17, 2024
64228dc
remove containerization
sarnex Dec 17, 2024
a5b980b
Merge remote-tracking branch 'upstream/main'
sarnex Dec 17, 2024
1c53aaa
remove --offload-arch changes
sarnex Dec 17, 2024
0eb76b4
Merge remote-tracking branch upstream/main
sarnex Dec 17, 2024
ee7ba68
missed stuff
sarnex Dec 17, 2024
261547c
dead code
sarnex Dec 17, 2024
9c97a92
remove invalid tests
sarnex Dec 17, 2024
1d927ee
Merge remote-tracking branch 'upstream/main'
sarnex Dec 30, 2024
eb408e9
fix test after libomptarget change
sarnex Dec 30, 2024
4764148
format
sarnex Dec 30, 2024
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
2 changes: 2 additions & 0 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1493,6 +1493,8 @@ def libomptarget_amdgcn_bc_path_EQ : Joined<["--"], "libomptarget-amdgcn-bc-path
HelpText<"Path to libomptarget-amdgcn bitcode library">, Alias<libomptarget_amdgpu_bc_path_EQ>;
def libomptarget_nvptx_bc_path_EQ : Joined<["--"], "libomptarget-nvptx-bc-path=">, Group<i_Group>,
HelpText<"Path to libomptarget-nvptx bitcode library">;
def libomptarget_spirv_bc_path_EQ : Joined<["--"], "libomptarget-spirv-bc-path=">, Group<i_Group>,
HelpText<"Path to libomptarget-spirv bitcode library">;
def dD : Flag<["-"], "dD">, Group<d_Group>, Visibility<[ClangOption, CC1Option]>,
HelpText<"Print macro definitions in -E mode in addition to normal output">;
def dI : Flag<["-"], "dI">, Group<d_Group>, Visibility<[ClangOption, CC1Option]>,
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Driver/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ add_clang_library(clangDriver
ToolChains/RISCVToolchain.cpp
ToolChains/Solaris.cpp
ToolChains/SPIRV.cpp
ToolChains/SPIRVOpenMP.cpp
ToolChains/TCE.cpp
ToolChains/UEFI.cpp
ToolChains/VEToolchain.cpp
Expand Down
40 changes: 35 additions & 5 deletions clang/lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "ToolChains/PS4CPU.h"
#include "ToolChains/RISCVToolchain.h"
#include "ToolChains/SPIRV.h"
#include "ToolChains/SPIRVOpenMP.h"
#include "ToolChains/Solaris.h"
#include "ToolChains/TCE.h"
#include "ToolChains/UEFI.h"
Expand Down Expand Up @@ -166,6 +167,20 @@ getHIPOffloadTargetTriple(const Driver &D, const ArgList &Args) {
return std::nullopt;
}

static std::optional<llvm::Triple>
getSPIRVOffloadTargetTriple(const Driver &D, const ArgList &Args) {
if (!Args.hasArg(options::OPT_offload_EQ))
return llvm::Triple(
"spirv64-intel"); // Only vendor "intel" is currently supported.
auto TT = getOffloadTargetTriple(D, Args);
if (!TT)
return std::nullopt;
if ((*TT).isSPIRV() && (*TT).getVendor() == llvm::Triple::Intel)
return TT;
D.Diag(diag::err_drv_invalid_or_unsupported_offload_target) << TT->str();
return std::nullopt;
}

// static
std::string Driver::GetResourcesPath(StringRef BinaryPath) {
// Since the resource directory is embedded in the module hash, it's important
Expand Down Expand Up @@ -888,11 +903,12 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
auto AMDTriple = getHIPOffloadTargetTriple(*this, C.getInputArgs());
auto NVPTXTriple = getNVIDIAOffloadTargetTriple(*this, C.getInputArgs(),
HostTC->getTriple());
auto SPIRVTriple = getSPIRVOffloadTargetTriple(*this, C.getInputArgs());

// Attempt to deduce the offloading triple from the set of architectures.
// We can only correctly deduce NVPTX / AMDGPU triples currently. We need
// to temporarily create these toolchains so that we can access tools for
// inferring architectures.
// We can only correctly deduce NVPTX / AMDGPU / SPIR-V triples currently.
// We need to temporarily create these toolchains so that we can access
// tools for inferring architectures.
llvm::DenseSet<StringRef> Archs;
if (NVPTXTriple) {
auto TempTC = std::make_unique<toolchains::CudaToolChain>(
Expand All @@ -908,7 +924,16 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
C, C.getArgs(), Action::OFK_OpenMP, &*TempTC, true))
Archs.insert(Arch);
}
if (!AMDTriple && !NVPTXTriple) {

if (SPIRVTriple) {
auto TempTC = std::make_unique<toolchains::SPIRVOpenMPToolChain>(
*this, *SPIRVTriple, *HostTC, C.getInputArgs());
for (StringRef Arch : getOffloadArchs(
C, C.getArgs(), Action::OFK_OpenMP, &*TempTC, true))
Archs.insert(Arch);
}

if (!AMDTriple && !NVPTXTriple && !SPIRVTriple) {
for (StringRef Arch :
getOffloadArchs(C, C.getArgs(), Action::OFK_OpenMP, nullptr, true))
Archs.insert(Arch);
Expand All @@ -922,6 +947,8 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
IsAMDOffloadArch(StringToOffloadArch(
getProcessorFromTargetID(*AMDTriple, Arch)))) {
DerivedArchs[AMDTriple->getTriple()].insert(Arch);
} else if (SPIRVTriple && Arch == (*SPIRVTriple).str()) {
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 Arch here exactly? My understanding is that SPIR-V has no concept of -mcpu arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah so here this is a case where SPIR-V is an outlier so stuff gets weird. This code gets hit when -fopenmp is passed, -fopenmp-targets is not passed, and --offload-arch is passed and we need to deduce the toolchain based on the offload arch. I implemented it such that you can pass spirv64-intel to --offload-arch and it will use the SPIR-V OpenMP toolchain. Arch will be spirv64-intel so we go inside the if and everything works.

Another option is that we could not allow the spirv64-intel option to --offload-arch to deduce to the SPIR-V OpenMP toolchain in this case, so basically you can't use --offload-arch with this toolchain. Maybe that's philosophically better.

Let me know what you think.

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 related to discussions I've had with @AlexVlx who is working on SPIR-V for HIP, but I strongly believe that we should deprecate -fopenmp-targets= and --offload-arch= should not be used for SPIR-V. We should fix-up the --offload= option to take the list of toolchains directly, and --offload-arch= should be used as a shorthand infereence. Then have -Xoffload-<triple> -arg to control specific arguments to toolchains.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, if that's is the direction we're going I can just remove this code so --offload-arch errors and you need -fopenmp-targets=spirv64-intel for now until the --offload option is updated.

DerivedArchs[Arch].insert(Arch);
} else {
Diag(clang::diag::err_drv_failed_to_deduce_target_from_arch) << Arch;
return;
Expand Down Expand Up @@ -962,7 +989,7 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
const ToolChain *TC;
// Device toolchains have to be selected differently. They pair host
// and device in their implementation.
if (TT.isNVPTX() || TT.isAMDGCN()) {
if (TT.isNVPTX() || TT.isAMDGCN() || TT.isSPIRV()) {
const ToolChain *HostTC =
C.getSingleOffloadToolChain<Action::OFK_Host>();
assert(HostTC && "Host toolchain should be always defined.");
Expand All @@ -975,6 +1002,9 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C,
else if (TT.isAMDGCN())
DeviceTC = std::make_unique<toolchains::AMDGPUOpenMPToolChain>(
*this, TT, *HostTC, C.getInputArgs());
else if (TT.isSPIRV())
DeviceTC = std::make_unique<toolchains::SPIRVOpenMPToolChain>(
*this, TT, *HostTC, C.getInputArgs());
else
assert(DeviceTC && "Device toolchain not defined.");
}
Expand Down
9 changes: 6 additions & 3 deletions clang/lib/Driver/ToolChains/CommonArgs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2829,10 +2829,13 @@ void tools::addOpenMPDeviceRTL(const Driver &D,
LibraryPaths.emplace_back(LibPath);

OptSpecifier LibomptargetBCPathOpt =
Triple.isAMDGCN() ? options::OPT_libomptarget_amdgpu_bc_path_EQ
: options::OPT_libomptarget_nvptx_bc_path_EQ;
Triple.isAMDGCN() ? options::OPT_libomptarget_amdgpu_bc_path_EQ
Copy link
Contributor

Choose a reason for hiding this comment

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

Does SPIR-V still rely on the mlink-builtin-bitcode hack instead of LTO? The SPIR-V backend should be usable soon from what I hear, so that's the ideal going forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we're linking the DeviceRTL using mlink-builtin-bitcode today, but we definitely want to use LTO in the future, both for OpenMP and SYCL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a patch for the DeviceRTL build? I've been meaning to gut a lot of that starting with #119091 and soon removing the whole bundling thing so it's more like libc.a. ideally we'd have lib/spirv64-intel-unknown/libomp.a and the driver just passes -Xoffload-linker -lomp.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, we hit some issues in the front end generating OpenMP device code with the SPIR-V target, so we can't build it yet because it doesn't make it through the compiler. Any change should be with fine with us, we should be able to adapt to whatever infrastructure there is to build the DeviceRTL. thanks.

: Triple.isNVPTX() ? options::OPT_libomptarget_nvptx_bc_path_EQ
: options::OPT_libomptarget_spirv_bc_path_EQ;

StringRef ArchPrefix = Triple.isAMDGCN() ? "amdgpu" : "nvptx";
StringRef ArchPrefix = Triple.isAMDGCN() ? "amdgpu"
: Triple.isNVPTX() ? "nvptx"
: "spirv64";
std::string LibOmpTargetName =
("libomptarget-" + ArchPrefix + "-" + BitcodeSuffix + ".bc").str();

Expand Down
5 changes: 4 additions & 1 deletion clang/lib/Driver/ToolChains/SPIRV.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ void SPIRV::constructTranslateCommand(Compilation &C, const Tool &T,

if (Input.getType() == types::TY_PP_Asm)
CmdArgs.push_back("-to-binary");

// The text output from spirv-dis is not in the format expected
// by llvm-spirv, so use the text output from llvm-spirv.
if (Output.getType() == types::TY_PP_Asm)
CmdArgs.push_back("--spirv-tools-dis");
CmdArgs.push_back("--spirv-text");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a separate fix most likely.

Copy link
Member Author

@sarnex sarnex Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, will drop this, thanks.

#120266


CmdArgs.append({"-o", Output.getFilename()});

Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Driver/ToolChains/SPIRV.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class LLVM_LIBRARY_VISIBILITY Linker final : public Tool {

namespace toolchains {

class LLVM_LIBRARY_VISIBILITY SPIRVToolChain final : public ToolChain {
class LLVM_LIBRARY_VISIBILITY SPIRVToolChain : public ToolChain {
mutable std::unique_ptr<Tool> Translator;

public:
Expand Down
38 changes: 38 additions & 0 deletions clang/lib/Driver/ToolChains/SPIRVOpenMP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//==- SPIRVOpenMP.cpp - SPIR-V OpenMP Tool Implementations --------*- C++ -*==//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//==------------------------------------------------------------------------==//
#include "SPIRVOpenMP.h"
#include "CommonArgs.h"

using namespace clang::driver;
using namespace clang::driver::toolchains;
using namespace clang::driver::tools;
using namespace llvm::opt;

namespace clang::driver::toolchains {
SPIRVOpenMPToolChain::SPIRVOpenMPToolChain(const Driver &D,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we truly need a separate OpenMP version of the toolchain? I don't know the details of SPIR-V or Intel's stack. It's relevant if we want OpenMP pulling in different libraries or something, like with AMDGPU's OpenMP skipping some of the HIP libraries.

Copy link
Member Author

@sarnex sarnex Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right now the only thing we are doing here is pulling in the OpenMP device RTL, but in the future I expect the final version of Intel's OpenMP support to have somewhat complicated logic here pulling in various things, and other vendors could also have some custom stuff, so I think it makes sense to have a new toolchain for now to not bog down the generic SPIR-V toolchain.

const llvm::Triple &Triple,
const ToolChain &HostToolchain,
const ArgList &Args)
: SPIRVToolChain(D, Triple, Args), HostTC(HostToolchain) {}

void SPIRVOpenMPToolChain::addClangTargetOptions(
const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadingKind) const {

if (DeviceOffloadingKind != Action::OFK_OpenMP)
return;

if (DriverArgs.hasArg(options::OPT_nogpulib))
return;
std::string GpuArch =
Twine(getTriple().getArchName() + "-" + getTriple().getVendorName())
.str();
addOpenMPDeviceRTL(getDriver(), DriverArgs, CC1Args, GpuArch, getTriple(),
HostTC);
}
} // namespace clang::driver::toolchains
29 changes: 29 additions & 0 deletions clang/lib/Driver/ToolChains/SPIRVOpenMP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//===--- SPIRVOpenMP.h - SPIR-V OpenMP Tool Implementations ------*- C++-*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_SPIRV_OPENMP_H
#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_SPIRV_OPENMP_H

#include "SPIRV.h"
#include "clang/Driver/Tool.h"
#include "clang/Driver/ToolChain.h"

namespace clang::driver::toolchains {
class LLVM_LIBRARY_VISIBILITY SPIRVOpenMPToolChain : public SPIRVToolChain {
public:
SPIRVOpenMPToolChain(const Driver &D, const llvm::Triple &Triple,
const ToolChain &HostTC, const llvm::opt::ArgList &Args);

void addClangTargetOptions(
const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadingKind) const override;

const ToolChain &HostTC;
};
} // namespace clang::driver::toolchains
#endif
1 change: 1 addition & 0 deletions clang/lib/Frontend/CompilerInvocation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4256,6 +4256,7 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args,

if (TT.getArch() == llvm::Triple::UnknownArch ||
!(TT.getArch() == llvm::Triple::aarch64 || TT.isPPC() ||
TT.getArch() == llvm::Triple::spirv64 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly the more I see added to this big if statement the more I'm convinced it doesn't need to exist.

Copy link
Member Author

@sarnex sarnex Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right now it's at least throwing an error if the triple doesn't match the list, so there might be some case where removing the error will break something, so I'd prefer to not potentially introduce a bug in a possibly untested area.

TT.getArch() == llvm::Triple::systemz ||
TT.getArch() == llvm::Triple::nvptx ||
TT.getArch() == llvm::Triple::nvptx64 ||
Expand Down
Empty file added clang/test/Driver/Inputs/spirv-openmp/lib/libomptarget-spirv64-spirv64-intel.bc
View file
Open in desktop
Empty file.
71 changes: 71 additions & 0 deletions clang/test/Driver/spirv-openmp-toolchain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// RUN: %clang -### --target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=spirv64-intel \
// RUN: --libomptarget-spirv-bc-path=%t/ -nogpulib %s 2>&1 \
// RUN: | FileCheck %s
// RUN: %clang -### --target=x86_64-unknown-linux-gnu -fopenmp --offload-arch=spirv64-intel \
// RUN: --libomptarget-spirv-bc-path=%t/ -nogpulib %s 2>&1 \
// RUN: | FileCheck %s

// verify the tools invocations
// CHECK: "-cc1" "-triple" "x86_64-unknown-linux-gnu"{{.*}}"-emit-llvm-bc"{{.*}}"-x" "c"
// CHECK: "-cc1" "-triple" "spirv64-intel" "-aux-triple" "x86_64-unknown-linux-gnu"
// CHECK: llvm-spirv{{.*}}
// CHECK: "-cc1" "-triple" "x86_64-unknown-linux-gnu"{{.*}}"-emit-obj"
// CHECK: clang-linker-wrapper{{.*}} "-o" "a.out"

// RUN: %clang -ccc-print-phases --target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=spirv64-intel %s 2>&1 \
// RUN: | FileCheck --check-prefix=CHECK-PHASES %s

// CHECK-PHASES: 0: input, "[[INPUT:.+]]", c, (host-openmp)
// CHECK-PHASES: 1: preprocessor, {0}, cpp-output, (host-openmp)
// CHECK-PHASES: 2: compiler, {1}, ir, (host-openmp)
// CHECK-PHASES: 3: input, "[[INPUT]]", c, (device-openmp)
// CHECK-PHASES: 4: preprocessor, {3}, cpp-output, (device-openmp)
// CHECK-PHASES: 5: compiler, {4}, ir, (device-openmp)
// CHECK-PHASES: 6: offload, "host-openmp (x86_64-unknown-linux-gnu)" {2}, "device-openmp (spirv64-intel)" {5}, ir
// CHECK-PHASES: 7: backend, {6}, assembler, (device-openmp)
// CHECK-PHASES: 8: assembler, {7}, object, (device-openmp)
// CHECK-PHASES: 9: offload, "device-openmp (spirv64-intel)" {8}, object
// CHECK-PHASES: 10: clang-offload-packager, {9}, image, (device-openmp)
// CHECK-PHASES: 11: offload, "host-openmp (x86_64-unknown-linux-gnu)" {2}, "device-openmp (x86_64-unknown-linux-gnu)" {10}, ir
// CHECK-PHASES: 12: backend, {11}, assembler, (host-openmp)
// CHECK-PHASES: 13: assembler, {12}, object, (host-openmp)
// CHECK-PHASES: 14: clang-linker-wrapper, {13}, image, (host-openmp)

// RUN: %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings -fopenmp -fopenmp-targets=spirv64-intel -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-BINDINGS
// RUN: %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings -fopenmp -fopenmp-targets=spirv64-intel -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-BINDINGS

// CHECK-BINDINGS: "x86_64-unknown-linux-gnu" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[HOST_BC:.+]]"
// CHECK-BINDINGS: "spirv64-intel" - "clang", inputs: ["[[INPUT]]", "[[HOST_BC]]"], output: "[[DEVICE_TEMP_BC:.+]]"
// CHECK-BINDINGS: "spirv64-intel" - "SPIR-V::Translator", inputs: ["[[DEVICE_TEMP_BC]]"], output: "[[DEVICE_SPV:.+]]"
// CHECK-BINDINGS: "x86_64-unknown-linux-gnu" - "Offload::Packager", inputs: ["[[DEVICE_SPV]]"], output: "[[DEVICE_IMAGE:.+]]"
// CHECK-BINDINGS: "x86_64-unknown-linux-gnu" - "clang", inputs: ["[[HOST_BC]]", "[[DEVICE_IMAGE]]"], output: "[[HOST_OBJ:.+]]"
// CHECK-BINDINGS: "x86_64-unknown-linux-gnu" - "Offload::Linker", inputs: ["[[HOST_OBJ]]"], output: "a.out"

// RUN: %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings -save-temps -fopenmp -fopenmp-targets=spirv64-intel -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-BINDINGS-TEMPS
// RUN: %clang -### --target=x86_64-unknown-linux-gnu -ccc-print-bindings -save-temps -fopenmp -fopenmp-targets=spirv64-intel %s 2>&1 | FileCheck %s --check-prefix=CHECK-BINDINGS-TEMPS
// CHECK-BINDINGS-TEMPS: "x86_64-unknown-linux-gnu" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[HOST_PP:.+]]"
// CHECK-BINDINGS-TEMPS: "x86_64-unknown-linux-gnu" - "clang", inputs: ["[[HOST_PP]]"], output: "[[HOST_BC:.+]]"
// CHECK-BINDINGS-TEMPS: "spirv64-intel" - "clang", inputs: ["[[INPUT]]"], output: "[[DEVICE_PP:.+]]"
// CHECK-BINDINGS-TEMPS: "spirv64-intel" - "clang", inputs: ["[[DEVICE_PP]]", "[[HOST_BC]]"], output: "[[DEVICE_TEMP_BC:.+]]"
// CHECK-BINDINGS-TEMPS: "spirv64-intel" - "SPIR-V::Translator", inputs: ["[[DEVICE_TEMP_BC]]"], output: "[[DEVICE_ASM:.+]]"
// CHECK-BINDINGS-TEMPS: "spirv64-intel" - "SPIR-V::Translator", inputs: ["[[DEVICE_ASM]]"], output: "[[DEVICE_SPV:.+]]"
// CHECK-BINDINGS-TEMPS: "x86_64-unknown-linux-gnu" - "Offload::Packager", inputs: ["[[DEVICE_SPV]]"], output: "[[DEVICE_IMAGE:.+]]"
// CHECK-BINDINGS-TEMPS: "x86_64-unknown-linux-gnu" - "clang", inputs: ["[[HOST_BC]]", "[[DEVICE_IMAGE]]"], output: "[[HOST_ASM:.+]]"
// CHECK-BINDINGS-TEMPS: "x86_64-unknown-linux-gnu" - "clang::as", inputs: ["[[HOST_ASM]]"], output: "[[HOST_OBJ:.+]]"
// CHECK-BINDINGS-TEMPS: "x86_64-unknown-linux-gnu" - "Offload::Linker", inputs: ["[[HOST_OBJ]]"], output: "a.out"

// RUN: %clang -### --target=x86_64-unknown-linux-gnu -emit-llvm -S -fopenmp -fopenmp-targets=spirv64-intel -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-EMIT-LLVM-IR
// CHECK-EMIT-LLVM-IR: "-cc1" "-triple" "spirv64-intel"{{.*}}"-emit-llvm-bc"

// RUN: %clang -### --target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=spirv64-intel \
// RUN: --sysroot=%S/Inputs/spirv-openmp/ %s 2>&1 | FileCheck --check-prefix=CHECK-GPULIB %s
// CHECK-GPULIB: "-cc1" "-triple" "spirv64-intel"{{.*}}"-mlink-builtin-bitcode" "{{.*}}libomptarget-spirv64-spirv64-intel.bc"

// RUN: not %clang -### -target x86_64-pc-linux-gnu -fopenmp --offload-arch=spirv64-intel,spirv64-unknown-unknown -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-TARGET-ID-ERROR
// CHECK-TARGET-ID-ERROR: error: failed to deduce triple for target architecture 'spirv64-unknown-unknown'

// RUN: not %clang -### -target x86_64-pc-linux-gnu -fopenmp --offload-arch=spirv64-intel,spirv64 -nogpulib %s 2>&1 | FileCheck %s --check-prefix=CHECK-TARGET-ID-ERROR-2
// CHECK-TARGET-ID-ERROR-2: error: failed to deduce triple for target architecture 'spirv64'

// RUN: not %clang -target x86_64-pc-linux-gnu -fopenmp --offload-arch=spirv64-intel -nogpulib --offload=spir64 %s 2>&1 | FileCheck %s --check-prefix=CHECK-TARGET-ID-ERROR-3
// CHECK-TARGET-ID-ERROR-3: error: invalid or unsupported offload target: 'spir64'
6 changes: 3 additions & 3 deletions clang/test/Driver/spirv-toolchain.cl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

// SPT64: "-cc1" "-triple" "spirv64"
// SPT64-SAME: "-o" [[BC:".*bc"]]
// SPT64: {{llvm-spirv.*"}} [[BC]] "--spirv-tools-dis" "-o" {{".*s"}}
// SPT64: {{llvm-spirv.*"}} [[BC]] "--spirv-text" "-o" {{".*s"}}

// RUN: %clang -### --target=spirv32 -x cl -S %s 2>&1 | FileCheck --check-prefix=SPT32 %s
// RUN: %clang -### --target=spirv32 -x ir -S %s 2>&1 | FileCheck --check-prefix=SPT32 %s
Expand All @@ -37,7 +37,7 @@

// SPT32: "-cc1" "-triple" "spirv32"
// SPT32-SAME: "-o" [[BC:".*bc"]]
// SPT32: {{llvm-spirv.*"}} [[BC]] "--spirv-tools-dis" "-o" {{".*s"}}
// SPT32: {{llvm-spirv.*"}} [[BC]] "--spirv-text" "-o" {{".*s"}}

//-----------------------------------------------------------------------------
// Check assembly input -> object output
Expand All @@ -55,7 +55,7 @@
// TMP: "-cc1" "-triple" "spirv64"
// TMP-SAME: "-o" [[BC:".*bc"]]
// TMP-SAME: [[I]]
// TMP: {{llvm-spirv.*"}} [[BC]] "--spirv-tools-dis" "-o" [[S:".*s"]]
// TMP: {{llvm-spirv.*"}} [[BC]] "--spirv-text" "-o" [[S:".*s"]]
// TMP: {{llvm-spirv.*"}} [[S]] "-to-binary" "-o" {{".*o"}}

//-----------------------------------------------------------------------------
Expand Down
17 changes: 11 additions & 6 deletions clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,14 +504,14 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args) {
{"-Xlinker",
Args.MakeArgString("--plugin-opt=" + StringRef(Arg->getValue()))});

if (!Triple.isNVPTX())
if (!Triple.isNVPTX() && !Triple.isSPIRV())
CmdArgs.push_back("-Wl,--no-undefined");

for (StringRef InputFile : InputFiles)
CmdArgs.push_back(InputFile);

// If this is CPU offloading we copy the input libraries.
if (!Triple.isAMDGPU() && !Triple.isNVPTX()) {
if (!Triple.isAMDGPU() && !Triple.isNVPTX() && !Triple.isSPIRV()) {
CmdArgs.push_back("-Wl,-Bsymbolic");
CmdArgs.push_back("-shared");
ArgStringList LinkerArgs;
Expand Down Expand Up @@ -595,6 +595,7 @@ Expected<StringRef> linkDevice(ArrayRef<StringRef> InputFiles,
case Triple::aarch64_be:
case Triple::ppc64:
case Triple::ppc64le:
case Triple::spirv64:
Copy link
Contributor

Choose a reason for hiding this comment

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

Some recent changes here for looongarch might need to rebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do, thanks

case Triple::systemz:
return generic::clang(InputFiles, Args);
default:
Expand Down Expand Up @@ -735,11 +736,15 @@ wrapDeviceImages(ArrayRef<std::unique_ptr<MemoryBuffer>> Buffers,
}

Expected<SmallVector<std::unique_ptr<MemoryBuffer>>>
bundleOpenMP(ArrayRef<OffloadingImage> Images) {
bundleOpenMP(SmallVectorImpl<OffloadingImage> &Images) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the container need to be mutable now?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's mutable because we modify it inside bundleOpenMP if the vendor is Intel, where we call containerizeOpenMPSPIRVImage to put the device image into an ELF (which we can't do using clang because there's no SPIR-V linker). I agree it's not ideal to have to modify the image here, if you have a better spot to do that ELF packaging, let me know. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

But this function's job is to return a new list of binary files after serializing the input, can't it just be put in the output?

Copy link
Member Author

@sarnex sarnex Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Somehow I convinced myself it might be more performant to do it this way, but I'm not so sure that's true after all, let me try to just modify the serialized input, thanks for the suggestion.

SmallVector<std::unique_ptr<MemoryBuffer>> Buffers;
for (const OffloadingImage &Image : Images)
for (OffloadingImage &Image : Images) {
llvm::Triple Triple(Image.StringData.lookup("triple"));
if (Triple.isSPIRV() && Triple.getVendor() == llvm::Triple::Intel)
offloading::intel::containerizeOpenMPSPIRVImage(Image);
Buffers.emplace_back(
MemoryBuffer::getMemBufferCopy(OffloadBinary::write(Image)));
}

return std::move(Buffers);
}
Expand Down Expand Up @@ -793,8 +798,8 @@ bundleHIP(ArrayRef<OffloadingImage> Images, const ArgList &Args) {
/// Transforms the input \p Images into the binary format the runtime expects
/// for the given \p Kind.
Expected<SmallVector<std::unique_ptr<MemoryBuffer>>>
bundleLinkedOutput(ArrayRef<OffloadingImage> Images, const ArgList &Args,
OffloadKind Kind) {
bundleLinkedOutput(SmallVectorImpl<OffloadingImage> &Images,
const ArgList &Args, OffloadKind Kind) {
llvm::TimeTraceScope TimeScope("Bundle linked output");
switch (Kind) {
case OFK_OpenMP:
Expand Down
5 changes: 5 additions & 0 deletions llvm/include/llvm/Frontend/Offloading/Utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ Error getAMDGPUMetaDataFromImage(MemoryBufferRef MemBuffer,
StringMap<AMDGPUKernelMetaData> &KernelInfoMap,
uint16_t &ELFABIVersion);
} // namespace amdgpu
namespace intel {
/// Containerizes an offloading image into the ELF binary format expected by
/// the Intel runtime offload plugin.
void containerizeOpenMPSPIRVImage(object::OffloadBinary::OffloadingImage &Img);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a separate patch with a unit test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will drop this for now and address your other feedback in a separate patch, thanks.

} // namespace intel
} // namespace offloading
} // namespace llvm

Expand Down
3 changes: 2 additions & 1 deletion llvm/include/llvm/TargetParser/Triple.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ class Triple {
Mesa,
SUSE,
OpenEmbedded,
LastVendorType = OpenEmbedded
Intel,
Copy link
Contributor

Choose a reason for hiding this comment

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

This really needs to be a separate patch.

Copy link
Member Author

@sarnex sarnex Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will do in #120250, also my bad I noticed I didn't even add a unit test for this here, fixed in new PR.

LastVendorType = Intel
};
enum OSType {
UnknownOS,
Expand Down
Loading
Loading