[SYCL][PI] Fix PI unittests and always build them #585

bjoernknafla · 2019-09-02T16:27:32Z

Enforce building of SYCL PI API unittests by default to catch build
problems due to CMake changes.

The PI API unittests use LLVM's add_unittest infrastructure which
only builds unittests if the CMake flag LLVM_BUILD_TESTS is ON.
The fix always sets this CMake variable locally when adding PI API
unittests.

This commit also contains other fixes required to build PI API
unittests again.

Signed-off-by: Bjoern Knafla [email protected]

nyalloc

Looks good to me.

Ruyk

Thanks, I think is important the PI API is always tested !

bjoernknafla · 2019-09-02T16:44:04Z

@Ruyk With the change it will be build by default but it is not part of any check-... tests, so no testing will happen automatically. Testing requires calling the built tools/sycl/unittests/pi/PiTests executable.

sycl/unittests/CMakeLists.txt

buildbot/check.py

buildbot/compile.py

buildbot/configure.py

bjoernknafla · 2019-09-04T15:18:02Z

sycl/CMakeLists.txt

-add_subdirectory( tools )
+if (NOT DEFINED LLVM_INCLUDE_TESTS)
+  set(LLVM_INCLUDE_TESTS ON)
+endif()


Aligns testing and unittests with other LLVM projects CMake usage.

Add this as a comment too.

The commit contains a comment that SYCL CMake is now stronger aligned with how the other LLVM projects are set up.

Other LLVM projects I checked do not comment on where they copied and adapted from other LLVM projects though it is clear that they are aligned.

bjoernknafla · 2019-09-04T15:18:15Z

sycl/unittests/CMakeLists.txt

-add_custom_target(PiUnitTests)
-set_target_properties(PiUnitTests PROPERTIES FOLDER "Tests")
+add_custom_target(SYCLUnitTests)
+set_target_properties(SYCLUnitTests PROPERTIES FOLDER "SYCL tests")


Aligns testing and unittests with other LLVM projects CMake usage, e.g., Clang

Other LLVM projects I checked do not comment on where they copied and adapted from other LLVM projects though it is clear that they are aligned.

buildbot/check.py

sycl/unittests/pi/CMakeLists.txt

buildbot/check.py

keryell

Thank you for the clarification!

buildbot/configure.py

keryell · 2019-09-06T19:17:27Z

sycl/CMakeLists.txt

-add_subdirectory( tools )
+if (NOT DEFINED LLVM_INCLUDE_TESTS)
+  set(LLVM_INCLUDE_TESTS ON)
+endif()


Add this as a comment too.

keryell · 2019-09-06T19:17:38Z

sycl/unittests/CMakeLists.txt

-add_custom_target(PiUnitTests)
-set_target_properties(PiUnitTests PROPERTIES FOLDER "Tests")
+add_custom_target(SYCLUnitTests)
+set_target_properties(SYCLUnitTests PROPERTIES FOLDER "SYCL tests")


buildbot/check.py

bjoernknafla · 2019-10-07T15:25:15Z

sycl/test/Unit/lit.site.cfg.py.in

@@ -0,0 +1,29 @@
+@LIT_SITE_CFG_IN_HEADER@


Copied and adapted from Clang.

bjoernknafla · 2019-10-07T15:25:27Z

sycl/test/Unit/lit.cfg.py

@@ -0,0 +1,59 @@
+# -*- Python -*-


Copied and adapted from Clang.

bjoernknafla · 2019-10-07T15:25:59Z

sycl/test/CMakeLists.txt

-  ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in
-  ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
+  ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
+  ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg.py


Aligning with Clang.

bjoernknafla · 2019-10-08T15:22:43Z

buildbot/sycl-ubu-x64-pr does not run - do I need to do something specific to trigger it?

smaslov-intel · 2019-10-09T03:39:53Z

Great thanks for doing this! The PI tests will be build always, but when/how are they run?

bjoernknafla · 2019-10-09T10:58:49Z

Great thanks for doing this! The PI tests will be build always, but when/how are they run?

@smaslov-intel This now works as in Clang and the other LLVM projects:

When SYCL testing is enabled with the CMake flag SYCL_INCLUDE_TESTS then the unittests are added as a dependency to the LIT tests (see CMake variable SYCL_TEST_DEPS).
This in itself ensures that the unittests are build when the other LIT tests are build, e.g., by running the build for the check-sycl target.
The LIT tests folder now has a Unit sub-folder which contains LIT config files that are gtest aware and are pointed to the unittests build directory to discover all included test executables to run.

bjoernknafla · 2019-10-09T11:01:27Z

I have addressed the changes requested by @bader and resolved the discussion though GitHub has not marked the change request as resolved.

smaslov-intel · 2019-10-11T04:33:40Z

Great thanks for doing this! The PI tests will be build always, but when/how are they run?

@smaslov-intel This now works as in Clang and the other LLVM projects:

When SYCL testing is enabled with the CMake flag SYCL_INCLUDE_TESTS then the unittests are added as a dependency to the LIT tests (see CMake variable SYCL_TEST_DEPS).

This in itself ensures that the unittests are build when the other LIT tests are build, e.g., by running the build for the check-sycl target.

The LIT tests folder now has a Unit sub-folder which contains LIT config files that are gtest aware and are pointed to the unittests build directory to discover all included test executables to run.

Thanks for the detailed explanation, but I still have these questions:

is there any way to run (not only build) these PI tests during a build, how?
how to run them outside of any build?

Comment is resolved.

bjoernknafla · 2019-10-11T11:18:41Z

1. is there any way to run (not only build) these PI tests during a build, how?

After configuring the project CMake (for example with the buildbot config script that now sets SYCL_INCLUDE_TEST=ON) run ninja check-sycl which will build and run the unittests as part of the check-sycl target. (Replace ninja with the build system of your choice or use CMake to run it, e.g., cmake --build /path/to/build/dir --target check-sycl

To only build and run the SYCL unittests use: ninja check-sycl-unit.

2. how to run them outside of any build?

Four ways (I know of):

After a build run ninja check-sycl or ninja check-all. As the build has already been done the LIT tests will be run directly and the unittests are now part of the check-sycl target LIT test run.
Only run the SYCL unittests after a build by calling ninja check-sycl-unit.
Run LIT by hand, e.g., from inside the build directory:

./bin/llvm-lit ./tools/sycl/test/Unit or
./bin/llvm-lit --filter .*SYCL-Unit.* ./tools/sycl/test

Run the unittests executable directly, e.g., from the build directory:

./tools/sycl/unittests/pi/PiTests

bjoernknafla · 2019-10-11T19:29:07Z

1. is there any way to run (not only build) these PI tests during a build, how?
After configuring the project CMake (for example with the buildbot config script that now sets SYCL_INCLUDE_TEST=ON) run ninja check-sycl which will build and run the unittests as part of the check-sycl target. (Replace ninja with the build system of your choice or use CMake to run it, e.g., cmake --build /path/to/build/dir --target check-sycl

There is no target to only build and run the SYCL unittests which is in-line with for example the Clang unittests.

I just noticed that this is not correct: for LLVM and Clang (and some other LLVM projects) special unit test targets exist, e.g.: check-llvm-unit and check-clang-unit - though I am not sure where these targets come from or how they are created...

bjoernknafla · 2019-10-11T19:41:57Z

I just noticed that this is not correct: for LLVM and Clang (and some other LLVM projects) special unit test targets exist, e.g.: check-llvm-unit and check-clang-unit - though I am not sure where these targets come from or how they are created...

I found it. I will push an update that enables a target called check-sycl-unit to build and run all SYCL unittests.

This update will also add build and run targets for all individual LIT tests of SYCL that you can list by doing the following on the command line:

ninja -t targets | grep check-sycl-

LLVM has two CMake options to include/build unittests: * `LLVM_INCLUDE_TESTS` to generate CMake targets for all kind of testing, which is `ON` by default. * `LLVM_BUILD_TESTS` to build the included unittests CMake targets, which is `OFF` by default. Other LLVM projects introduce a `<PROJECT>_INCLUDE_TESTS` option to control if CMake targets should be created for their different tests. If their unittests are build by default is controlled by the `LLVM_BUILD_TESTS` option. This commit aligns SYCL testing and unittesting with other LLVM projects, e.g.: * introduces a `SYCL_INCLUDE_TESTS` option, * renames the SYCL unittest suite to `SYCLUnitTests`, and * introduces the `add_sycl_unittest` CMake function to simplify adding SYCL-specific unittests. * Aligns naming of CMake unittest folder with Clang. * Copies Clang unittest LIT infrastructure to SYCL unittests. * Renames LIT testsuite names of SYCL to "SYCL" and * names the unittests in LIT as "SYCL-Unit". * Adapts other LLVM projects LIT config files naming scheme. * Cleans up CMake variables used by LIT config `.in` files and aligns variable names stronger with other LLVM projects. * Makes the SYCL unittests part of the `check-sycl` target, i.e., builds them on demand. * Adds a CMake target to build/run every SYLC LIT test including unit tests with `check-sycl-unit`. Signed-off-by: Bjoern Knafla <[email protected]>

SYCL examples, e.g., in the Khronos specs, include the SYCL header as system headers. Adapt includes in the `CL/sycl/detail/pi.hpp` header to this style. Signed-off-by: Bjoern Knafla <[email protected]>

bjoernknafla · 2019-10-11T21:16:37Z

sycl/test/CMakeLists.txt

  )
 set_target_properties(check-sycl PROPERTIES FOLDER "SYCL tests")
+
+add_lit_testsuites(SYCL ${CMAKE_CURRENT_SOURCE_DIR}


This adds a check-sycl-<subdir> target for every sub-directory of sycl/test including for the Unit sub-directory which will run the unittests from sycl/unittests.

bjoernknafla · 2019-10-11T21:17:06Z

sycl/test/lit.cfg.py

@@ -14,7 +14,7 @@
 # Configuration file for the 'lit' test runner.

 # name: The name of this test suite.
-config.name = 'SYCLUnitTests'
+config.name = 'SYCL'


This is in line with Clang now.

smaslov-intel · 2019-10-12T00:39:09Z

Thanks @bjoernknafla.
I have no further questions at this point.

Just a comment that in a longer run pi::initialize() can see multiple PI plugins, and hence there should be a way in these PI tests to check running through all of them (or the one that is somehow forced).

bjoernknafla · 2019-10-14T10:55:31Z

Just a comment that in a longer run pi::initialize() can see multiple PI plugins, and hence there should be a way in these PI tests to check running through all of them (or the one that is somehow forced).

Makes sense.

nyalloc self-requested a review September 2, 2019 16:29

nyalloc previously approved these changes Sep 2, 2019

View reviewed changes

Ruyk previously approved these changes Sep 2, 2019

View reviewed changes

bader previously requested changes Sep 2, 2019

View reviewed changes

sycl/unittests/CMakeLists.txt Outdated Show resolved Hide resolved

bjoernknafla dismissed stale reviews from Ruyk and nyalloc via be18556 September 4, 2019 15:08