[SYCL] Do not force LLVM_INCLUDE_TESTS variable #1505
Conversation
This variable must be set by the developers. Signed-off-by: Alexey Bader <[email protected]>
| @@ -228,15 +228,10 @@ if (SYCL_ENABLE_XPTI_TRACING) | |||
| endif() | |||
| endif() | |||
|
|
|||
| if (NOT DEFINED LLVM_INCLUDE_TESTS) | |||
| set(LLVM_INCLUDE_TESTS ON) | |||
There was a problem hiding this comment.
Let's discuss if we need this code or if we can replace it with something better.
This snippet was added by #585 and it seems to handle the case when SYCL runtime project is built separately from llvm. I don't think we currently support this case, so it can be safely removed.
If we need to support this case we should:
- add regular testing for this configuration
- handle it more accurately - e.g. like libcxx does here
There was a problem hiding this comment.
Agree with removing these lines. @bjoernknafla Do you see any reason why we should keep it?
There was a problem hiding this comment.
I have mirrored how Clang does it but am not sure if Clang is a special case compared to the other LLVM projects that deal differently with it, e.g., LLD.
As long as we ensure that SYCL_INCLUDE_TESTS is set in the buildbots (as it currently is I see no reason to remove it though.
It might surprise users though that suddenly the tests are off, and they might see tests pass because they aren’t tests, if they haven’t explicitly turned the SYCL ones on?
There was a problem hiding this comment.
According to my understanding, as long as SYCL project is build withing LLVM tree, we don't need to set SYCL_INCLUDE_TESTS explicitly.
I've tried it locally by removing this line, and test target is included.
This code handles the case if LLVM CMake files are not processed before SYCL CMake files and LLVM_INCLUDE_TESTS variable is not set. As I mentioned in the comments above, this configuration is not supported and we have a lot of other variables to set in addition to LLVM_INCLUDE_TESTS to use SYCL project in stand-alone mode.
@bjoernknafla, are you building SYCL project separately from LLVM?
There was a problem hiding this comment.
@bader I am not building SYCL separately from LLVM. In the future it would be helpful to enable separate building as this could speed up testing, e.g., a compiler change needs to build and run all tests, a runtime change only needs to build and run runtime tests - just a thought and nothing that could inform a variable change right now.
Currently SYCL_INCLUDE_TESTS is set automatically to LLVM_INCLUDE_TESTS: sycl/CMakeLists.txt#L244
How did you test after the CMake configuration change, e.g., did you use the buildbot scripts (which set the variable implicitly), and did you delete the CMake cache and run the configuration freshly? (Only asking to prevent turning tests off by accident which has bitten me in the past).
There was a problem hiding this comment.
Currently
SYCL_INCLUDE_TESTSis set automatically toLLVM_INCLUDE_TESTS: sycl/CMakeLists.txt#L244
LLVM_INCLUDE_TESTS default value is ON, so if sycl project is build within llvm tree, cmake parses LLVM project cmake files and set LLVM_INCLUDE_TESTS variable to ON (unless it's explicitly set to OFF by user. This means that SYCL_INCLUDE_TESTS default value is ON in our current configuration.
How did you test after the CMake configuration change, e.g., did you use the buildbot scripts (which set the variable implicitly), and did you delete the CMake cache and run the configuration freshly? (Only asking to prevent turning tests off by accident which has bitten me in the past).
I removed the whole build directly and removed the line 83 from the configure.py script. Then I run the script to generate Ninja files and run ninja check-sycl command, which works as expected.
There was a problem hiding this comment.
Thank you for helping me see your full picture. Sounds good to me.
…c_abi_checks * origin/sycl: (32 commits) [SYCL] Do not force LLVM_INCLUDE_TESTS variable (intel#1505) [SYCL][NFC] Align nd_item members with constructor initialization list (intel#1521) [SYCL] Move get_info_host implementation to header (intel#1514) [SYCL] Always use dynamic CRT for Unit tests (intel#1515) [SYCL][NFC] Temporarily disable sporadically failing test (intel#1526) [SYCL] Fix inline namespaces (intel#1525) [SYCL] Release notes for March'20 DPCPP implementation update (intel#1511) [SYCL][PI][CUDA] Implements get_native interoperability (intel#1332) [SYCL] Fix check-sycl test suite on systems w/o OpenCL (intel#1503) [SYCL][Doc] Update ExtendedAtomics documentation (intel#1487) [SYCL][CUDA] Expose context extended deleters on PI API (intel#1483) [SYCL][NFC] Remove a dropped environment variable from a test (intel#1506) [SYCL] Add opencl-aot to sycl-toolchain target (intel#1504) [SYCL] Allow to run deploy LIT tests from particular directory [SYCL][CUDA] Fix LIT testing with CUDA devices (intel#1300) [SYCL] Remove operator name keywords (intel#1501) [Driver][SYCL] Consider .lo files as static archives (intel#1500) [SYCL-PTX] Update the compiler design to describe the CUDA target (intel#1408) [SYCL] Fix library build on Windows (intel#1499) [SYCL][NFC] Refactor lit.cfg.py (intel#1452) ...
This variable must be set by the developers only.
Signed-off-by: Alexey Bader [email protected]