[Driver][SYCL][NFC] Update interface to access SYCL header additions #15302
Conversation
Move the ability to add the SYCL headers into the SYCLInstallation class to better conform to existing usage for other toolchains. Also improve the toolchain access for adding the headers using a virtual function.
mdtoguchi
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
clang/lib/Driver/ToolChains/SYCL.cpp
Outdated
| // ../include/sycl | ||
| // ../include/sycl/stl_wrappers | ||
| // ../include |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
I don't know about include/sycl vs include/, but the stl_wrappers path has to be added in the driver.
There was a problem hiding this comment.
Are there any conflicts between the files in ../include/sycl/stl_wrappers and ../include?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What exactly is "maintainability benefits"? Note, these are directories of deployed compiler. We can still have separate directories in the sources code.
There was a problem hiding this comment.
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.
mdtoguchi
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
mdtoguchi
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
mdtoguchi
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/dpcpp-clang-driver-reviewers, please take a look - thanks! |
|
@intel/llvm-gatekeepers, this should be ready for merge - thanks! |
| CC1Args.push_back("-internal-isystem"); | ||
| CC1Args.push_back(DriverArgs.MakeArgString(IncludePath)); | ||
| void SYCLToolChain::AddSYCLIncludeArgs(const ArgList &DriverArgs, | ||
| ArgStringList &CC1Args) const { |
There was a problem hiding this comment.
Should this have the 'override' specifier?
There was a problem hiding this comment.
AFAIK, the override is not needed for the function definition of the derived class, as the declaration states the override.
mdtoguchi
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
mdtoguchi
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
mdtoguchi
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
mdtoguchi
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
Had to fix some conflicts, after update things are clean. @intel/llvm-gatekeepers can this be merged? |
6 participants
Move the ability to add the SYCL headers into the SYCLInstallation class to better conform to existing usage for other toolchains. Also improve the toolchain access for adding the headers using a virtual function.