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

[SYCL] Do not force LLVM_INCLUDE_TESTS variable #1505

Merged
merged 1 commit into from
Apr 15, 2020
+0 −7
Merged

[SYCL] Do not force LLVM_INCLUDE_TESTS variable #1505

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 0 additions & 7 deletions sycl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,15 +228,10 @@ if (SYCL_ENABLE_XPTI_TRACING)
endif()
endif()

if (NOT DEFINED LLVM_INCLUDE_TESTS)
set(LLVM_INCLUDE_TESTS ON)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. add regular testing for this configuration
  2. handle it more accurately - e.g. like libcxx does here

Copy link
Contributor

@romanovvlad romanovvlad Apr 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Agree with removing these lines. @bjoernknafla Do you see any reason why we should keep it?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently SYCL_INCLUDE_TESTS is set automatically to LLVM_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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for helping me see your full picture. Sounds good to me.

endif()

option(SYCL_INCLUDE_TESTS
"Generate build targets for the SYCL unit tests."
${LLVM_INCLUDE_TESTS})


# Plugin Library
add_subdirectory( plugins )

Expand Down Expand Up @@ -274,7 +269,6 @@ set( SYCL_TOOLCHAIN_DEPLOY_COMPONENTS
libsycldevice
)


if(SYCL_BUILD_PI_CUDA)
# Ensure that libclc is enabled.
list(FIND LLVM_ENABLE_PROJECTS libclc LIBCLC_FOUND)
Expand All @@ -287,7 +281,6 @@ if(SYCL_BUILD_PI_CUDA)
list(APPEND SYCL_TOOLCHAIN_DEPLOY_COMPONENTS libspirv-builtins pi_cuda)
endif()


# Use it as fake dependency in order to force another command(s) to execute.
add_custom_command(OUTPUT __force_it
COMMAND "${CMAKE_COMMAND}" -E echo
Expand Down