Skip to content

[Driver][SYCL][NFC] Update interface to access SYCL header additions #15302

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

Merged
merged 4 commits into from
Oct 11, 2024
Merged

[Driver][SYCL][NFC] Update interface to access SYCL header additions #15302

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7613f5a
[Driver][SYCL][NFC] Update interface to access SYCL header additions
mdtoguchi Sep 5, 2024
b4bd41c
Merge remote-tracking branch 'intel_llvm/sycl' into upstream-cleanup-3
mdtoguchi Oct 8, 2024
79ef94e
Fix bad merge
mdtoguchi Oct 8, 2024
a865550
Merge remote-tracking branch 'intel_llvm/sycl' into upstream-cleanup-3
mdtoguchi Oct 9, 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
4 changes: 4 additions & 0 deletions clang/include/clang/Driver/ToolChain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,10 @@ class ToolChain {
virtual void AddHIPIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const;

/// Add arguments to use SYCL specific includes.
virtual void AddSYCLIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const;

/// Add arguments to use MCU GCC toolchain includes.
virtual void AddIAMCUIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const;
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Driver/ToolChain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1477,6 +1477,9 @@ void ToolChain::AddCudaIncludeArgs(const ArgList &DriverArgs,
void ToolChain::AddHIPIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {}

void ToolChain::AddSYCLIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {}

llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12>
ToolChain::getDeviceLibs(
const ArgList &DriverArgs,
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
getToolChain().AddHIPIncludeArgs(Args, CmdArgs);

if (JA.isOffloading(Action::OFK_SYCL)) {
toolchains::SYCLToolChain::AddSYCLIncludeArgs(D, Args, CmdArgs);
getToolChain().AddSYCLIncludeArgs(Args, CmdArgs);
if (Inputs[0].getType() == types::TY_CUDA) {
// Include __clang_cuda_runtime_wrapper.h in .cu SYCL compilation.
getToolChain().AddCudaIncludeArgs(Args, CmdArgs);
Expand Down
8 changes: 3 additions & 5 deletions clang/lib/Driver/ToolChains/Cuda.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,7 @@ CudaToolChain::CudaToolChain(const Driver &D, const llvm::Triple &Triple,
const ToolChain &HostTC, const ArgList &Args,
const Action::OffloadKind OK)
: NVPTXToolChain(D, Triple, HostTC.getTriple(), Args), HostTC(HostTC),
OK(OK) {}
SYCLInstallation(D), OK(OK) {}

void CudaToolChain::addClangTargetOptions(
const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args,
Expand Down Expand Up @@ -956,8 +956,7 @@ void CudaToolChain::addClangTargetOptions(
}

if (DeviceOffloadingKind == Action::OFK_SYCL) {
toolchains::SYCLToolChain::AddSYCLIncludeArgs(getDriver(), DriverArgs,
CC1Args);
SYCLInstallation.AddSYCLIncludeArgs(DriverArgs, CC1Args);

if (DriverArgs.hasArg(options::OPT_fsycl_fp32_prec_sqrt)) {
CC1Args.push_back("-fcuda-prec-sqrt");
Expand Down Expand Up @@ -1190,8 +1189,7 @@ CudaToolChain::GetCXXStdlibType(const ArgList &Args) const {
void CudaToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
if (DriverArgs.hasArg(options::OPT_fsycl)) {
toolchains::SYCLToolChain::AddSYCLIncludeArgs(getDriver(), DriverArgs,
CC1Args);
SYCLInstallation.AddSYCLIncludeArgs(DriverArgs, CC1Args);
}
HostTC.AddClangSystemIncludeArgs(DriverArgs, CC1Args);

Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Driver/ToolChains/Cuda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ class LLVM_LIBRARY_VISIBILITY CudaToolChain : public NVPTXToolChain {
Tool *SelectTool(const JobAction &JA) const override;
const ToolChain &HostTC;

SYCLInstallationDetector SYCLInstallation;

protected:
Tool *buildAssembler() const override; // ptxas
Tool *buildLinker() const override; // fatbinary (ok, not really a linker)
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Driver/ToolChains/Gnu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3190,7 +3190,8 @@ bool Generic_GCC::GCCInstallationDetector::ScanGentooGccConfig(
Generic_GCC::Generic_GCC(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
: ToolChain(D, Triple, Args), GCCInstallation(D),
CudaInstallation(D, Triple, Args), RocmInstallation(D, Triple, Args) {
CudaInstallation(D, Triple, Args), RocmInstallation(D, Triple, Args),
SYCLInstallation(D) {
getProgramPaths().push_back(getDriver().Dir);
}

Expand Down
1 change: 1 addition & 0 deletions clang/lib/Driver/ToolChains/Gnu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ class LLVM_LIBRARY_VISIBILITY Generic_GCC : public ToolChain {
GCCInstallationDetector GCCInstallation;
LazyDetector<CudaInstallationDetector> CudaInstallation;
LazyDetector<RocmInstallationDetector> RocmInstallation;
SYCLInstallationDetector SYCLInstallation;

public:
Generic_GCC(const Driver &D, const llvm::Triple &Triple,
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/Driver/ToolChains/HIPAMD.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ void AMDGCN::Linker::ConstructJob(Compilation &C, const JobAction &JA,
HIPAMDToolChain::HIPAMDToolChain(const Driver &D, const llvm::Triple &Triple,
const ToolChain &HostTC, const ArgList &Args,
const Action::OffloadKind OK)
: ROCMToolChain(D, Triple, Args), HostTC(HostTC), OK(OK) {
: ROCMToolChain(D, Triple, Args), HostTC(HostTC), SYCLInstallation(D),
OK(OK) {
// Lookup binaries into the driver directory, this is used to
// discover the clang-offload-bundler executable.
getProgramPaths().push_back(getDriver().Dir);
Expand Down Expand Up @@ -318,8 +319,7 @@ void HIPAMDToolChain::addClangTargetOptions(
CC1Args.push_back("-fembed-bitcode=marker");

if (DeviceOffloadingKind == Action::OFK_SYCL) {
toolchains::SYCLToolChain::AddSYCLIncludeArgs(getDriver(), DriverArgs,
CC1Args);
SYCLInstallation.AddSYCLIncludeArgs(DriverArgs, CC1Args);
}

auto NoLibSpirv = DriverArgs.hasArg(options::OPT_fno_sycl_libspirv) ||
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Driver/ToolChains/HIPAMD.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ class LLVM_LIBRARY_VISIBILITY HIPAMDToolChain final : public ROCMToolChain {
void checkTargetID(const llvm::opt::ArgList &DriverArgs) const override;
Tool *SelectTool(const JobAction &JA) const override;

SYCLInstallationDetector SYCLInstallation;

protected:
Tool *buildLinker() const override;

Expand Down
5 changes: 5 additions & 0 deletions clang/lib/Driver/ToolChains/Linux.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,11 @@ void Linux::AddIAMCUIncludeArgs(const ArgList &DriverArgs,
}
}

void Linux::AddSYCLIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
SYCLInstallation.AddSYCLIncludeArgs(DriverArgs, CC1Args);
}

bool Linux::isPIEDefault(const llvm::opt::ArgList &Args) const {
return CLANG_DEFAULT_PIE_ON_LINUX || getTriple().isAndroid() ||
getTriple().isMusl() || getSanitizerArgs(Args).requiresPIE();
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Driver/ToolChains/Linux.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class LLVM_LIBRARY_VISIBILITY Linux : public Generic_ELF {
llvm::opt::ArgStringList &CmdArgs) const override;
void AddIAMCUIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
void AddSYCLIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
RuntimeLibType GetDefaultRuntimeLibType() const override;
unsigned GetDefaultDwarfVersion() const override;
CXXStdlibType GetDefaultCXXStdlibType() const override;
Expand Down
7 changes: 6 additions & 1 deletion clang/lib/Driver/ToolChains/MSVC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ void visualstudio::Linker::ConstructJob(Compilation &C, const JobAction &JA,
MSVCToolChain::MSVCToolChain(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
: ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args),
RocmInstallation(D, Triple, Args) {
RocmInstallation(D, Triple, Args), SYCLInstallation(D) {
getProgramPaths().push_back(getDriver().Dir);

std::optional<llvm::StringRef> VCToolsDir, VCToolsVersion;
Expand Down Expand Up @@ -586,6 +586,11 @@ void MSVCToolChain::AddHIPIncludeArgs(const ArgList &DriverArgs,
RocmInstallation->AddHIPIncludeArgs(DriverArgs, CC1Args);
}

void MSVCToolChain::AddSYCLIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
SYCLInstallation.AddSYCLIncludeArgs(DriverArgs, CC1Args);
}

void MSVCToolChain::AddHIPRuntimeLibArgs(const ArgList &Args,
ArgStringList &CmdArgs) const {
CmdArgs.append({Args.MakeArgString(StringRef("-libpath:") +
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Driver/ToolChains/MSVC.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ class LLVM_LIBRARY_VISIBILITY MSVCToolChain : public ToolChain {
void AddHIPRuntimeLibArgs(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs) const override;

void AddSYCLIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;

bool getWindowsSDKLibraryPath(
const llvm::opt::ArgList &Args, std::string &path) const;
bool getUniversalCRTLibraryPath(const llvm::opt::ArgList &Args,
Expand Down Expand Up @@ -144,6 +147,7 @@ class LLVM_LIBRARY_VISIBILITY MSVCToolChain : public ToolChain {
llvm::ToolsetLayout VSLayout = llvm::ToolsetLayout::OlderVS;
LazyDetector<CudaInstallationDetector> CudaInstallation;
LazyDetector<RocmInstallationDetector> RocmInstallation;
SYCLInstallationDetector SYCLInstallation;
};

} // end namespace toolchains
Expand Down
52 changes: 28 additions & 24 deletions clang/lib/Driver/ToolChains/SYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,30 @@ void SYCLInstallationDetector::getSYCLDeviceLibPath(
DeviceLibPaths.emplace_back(D.SysRoot + "/lib");
}

void SYCLInstallationDetector::AddSYCLIncludeArgs(
const ArgList &DriverArgs, ArgStringList &CC1Args) const {
// Add the SYCL header search locations in the specified order.
// ../include/sycl
// ../include/sycl/stl_wrappers
// ../include
Copy link
Contributor

@bader bader Sep 5, 2024

Choose a reason for hiding this comment

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

@intel/llvm-reviewers-runtime, I suggest we remove the need for the driver to add 3 different locations for SYCL headers restricted by specific order. Considering that we expose only one header to the user API - sycl/sycl.hpp, the driver can include only one location and runtime library project can put everything into a single location or include from other locations. Right?

Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 5, 2024

Choose a reason for hiding this comment

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

I don't know about include/sycl vs include/, but the stl_wrappers path has to be added in the driver.

Copy link
Contributor

@bader bader Sep 5, 2024

Choose a reason for hiding this comment

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

Are there any conflicts between the files in ../include/sycl/stl_wrappers and ../include?

Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 5, 2024

Choose a reason for hiding this comment

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

I believe maintainability benefits of having these headers in a separate dedicated folder outweigh the costs of having driver add one extra include path, if that's where you're going with your question.

Copy link
Contributor

@bader bader Sep 5, 2024

Choose a reason for hiding this comment

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

What exactly is "maintainability benefits"? Note, these are directories of deployed compiler. We can still have separate directories in the sources code.

Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 5, 2024

Choose a reason for hiding this comment

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

First, "smart" deploy is extra maintainability burden. Second, I want to be able to remove stl_wrappers from search path via manually modifying clang -cc1, merging them together with other includes would prevent that. I'm sure there are "third"/"fourth" if I were to think about it just a bit more.

SmallString<128> IncludePath(D.Dir);
llvm::sys::path::append(IncludePath, "..");
llvm::sys::path::append(IncludePath, "include");
SmallString<128> SYCLPath(IncludePath);
llvm::sys::path::append(SYCLPath, "sycl");
// This is used to provide our wrappers around STL headers that provide
// additional functions/template specializations when the user includes those
// STL headers in their programs (e.g., <complex>).
SmallString<128> STLWrappersPath(SYCLPath);
llvm::sys::path::append(STLWrappersPath, "stl_wrappers");
CC1Args.push_back("-internal-isystem");
CC1Args.push_back(DriverArgs.MakeArgString(SYCLPath));
CC1Args.push_back("-internal-isystem");
CC1Args.push_back(DriverArgs.MakeArgString(STLWrappersPath));
CC1Args.push_back("-internal-isystem");
CC1Args.push_back(DriverArgs.MakeArgString(IncludePath));
}

void SYCLInstallationDetector::print(llvm::raw_ostream &OS) const {
if (!InstallationCandidates.size())
return;
Expand Down Expand Up @@ -1389,7 +1413,7 @@ static std::vector<OptSpecifier> getUnsupportedOpts(void) {

SYCLToolChain::SYCLToolChain(const Driver &D, const llvm::Triple &Triple,
const ToolChain &HostTC, const ArgList &Args)
: ToolChain(D, Triple, Args), HostTC(HostTC),
: ToolChain(D, Triple, Args), HostTC(HostTC), SYCLInstallation(D),
IsSYCLNativeCPU(Triple == HostTC.getTriple()) {
// Lookup binaries into the driver directory, this is used to
// discover the clang-offload-bundler executable.
Expand Down Expand Up @@ -1824,29 +1848,9 @@ SYCLToolChain::GetCXXStdlibType(const ArgList &Args) const {
return HostTC.GetCXXStdlibType(Args);
}

void SYCLToolChain::AddSYCLIncludeArgs(const clang::driver::Driver &Driver,
const ArgList &DriverArgs,
ArgStringList &CC1Args) {
// Add the SYCL header search locations in the specified order.
// ../include/sycl
// ../include/sycl/stl_wrappers
// ../include
SmallString<128> IncludePath(Driver.Dir);
llvm::sys::path::append(IncludePath, "..");
llvm::sys::path::append(IncludePath, "include");
SmallString<128> SYCLPath(IncludePath);
llvm::sys::path::append(SYCLPath, "sycl");
// This is used to provide our wrappers around STL headers that provide
// additional functions/template specializations when the user includes those
// STL headers in their programs (e.g., <complex>).
SmallString<128> STLWrappersPath(SYCLPath);
llvm::sys::path::append(STLWrappersPath, "stl_wrappers");
CC1Args.push_back("-internal-isystem");
CC1Args.push_back(DriverArgs.MakeArgString(SYCLPath));
CC1Args.push_back("-internal-isystem");
CC1Args.push_back(DriverArgs.MakeArgString(STLWrappersPath));
CC1Args.push_back("-internal-isystem");
CC1Args.push_back(DriverArgs.MakeArgString(IncludePath));
void SYCLToolChain::AddSYCLIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
Copy link
Contributor

@srividya-sundaram srividya-sundaram Sep 18, 2024

Choose a reason for hiding this comment

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

Should this have the 'override' specifier?

Copy link
Contributor Author

@mdtoguchi mdtoguchi Sep 18, 2024

Choose a reason for hiding this comment

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

AFAIK, the override is not needed for the function definition of the derived class, as the declaration states the override.

SYCLInstallation.AddSYCLIncludeArgs(DriverArgs, CC1Args);
}

void SYCLToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
Expand Down
9 changes: 6 additions & 3 deletions clang/lib/Driver/ToolChains/SYCL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class SYCLInstallationDetector {
SYCLInstallationDetector(const Driver &D);
void getSYCLDeviceLibPath(
llvm::SmallVector<llvm::SmallString<128>, 4> &DeviceLibPaths) const;
void AddSYCLIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const;
void print(llvm::raw_ostream &OS) const;

private:
Expand Down Expand Up @@ -214,9 +216,8 @@ class LLVM_LIBRARY_VISIBILITY SYCLToolChain : public ToolChain {

void addClangWarningOptions(llvm::opt::ArgStringList &CC1Args) const override;
CXXStdlibType GetCXXStdlibType(const llvm::opt::ArgList &Args) const override;
static void AddSYCLIncludeArgs(const clang::driver::Driver &Driver,
const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args);
void AddSYCLIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
void AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const override;
void AddClangCXXStdlibIncludeArgs(
Expand All @@ -228,6 +229,8 @@ class LLVM_LIBRARY_VISIBILITY SYCLToolChain : public ToolChain {
const ToolChain &HostTC;
const bool IsSYCLNativeCPU;

SYCLInstallationDetector SYCLInstallation;

protected:
Tool *buildBackendCompiler() const override;
Tool *buildLinker() const override;
Expand Down