-
Notifications
You must be signed in to change notification settings - Fork 16k
[libsycl] Add lit configuration files and basic test #177407
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
✅ With the latest revision this PR passed the Python code formatter. |
24ab14b to
6a8d6d8
Compare
2533f18 to
6a8d6d8
Compare
|
Hi @tahonermann, @dvrogozh, @bader, @againull, @dm-vodopyanov FYI, published for review. a few notes: All extra features, parameters, configurations will be added as libsycl code base & feature coverage grow. Also adding @uditagarwal97 who has an experience with lit feature implementation in intel/llvm and who kindly agreed to make a review of this PR. P.S. this PR depends on device impl (#176972) to run lit (sycl-ls should be able to identify devices). check-sycl-e2e is not included into check-all so it won't break anything in buildbot for libsycl + liboffload if merged earlier. Once both PRs land in Thank you. |
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
6a8d6d8 to
904e8ff
Compare
libsycl/test_e2e/CMakeLists.txt
Outdated
| set(LIBSYCL_TEST_E2E_TARGETS "all") | ||
| endif() | ||
|
|
||
| if(NOT DEFINED LEVEL_ZERO_LIBS_DIR AND NOT DEFINED LEVEL_ZERO_INCLUDE_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I am confused. Per my understanding default way to find L0 should be via pkg-config. Do I understand it right that that's how L0 actually will be found in some upper level cmake list? And if it will be why this customization with LEVEL_ZERO_LIBS_DIR and LEVEL_ZERO_INCLUDE_DIR is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: removed that, it is not needed right now, I noted about PR that "And it has a little bit more than I use now." but since there are questions about this , I removed it.
the answer is: this code is not intended to find default L0 version, it was intended to provide interface to configure tests to some specific L0 setup/version, to override default one.
| ## Prerequisites | ||
|
|
||
| * Target runtime(s) to execute tests on devices. | ||
| TODO: add link to liboffload instruction once they add it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't liboffload just be there if you build clang/llvm with sycl support? I was expecting that here will be a list of runtimes such as L0 which must be available for the test to run. Should we list here these runtimes one by one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your understanding is correct and the idea is to have an instruction about L0/CUDA/HIP runtimes setup here. And I believe this instruction must be provided on liboffload level since it is the runtime that directly works with them. This instruction is absent since liboffload has poor (and/or absent) documentation now. Intel liboffload team has docs in TODO list and will add it once it will be ready.
libsycl/test_e2e/README.md
Outdated
| * **linux** - host OS; | ||
| * **any-device-is-gpu** - device type to be available; | ||
| * **any-device-is-level_zero** - backend to be available; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * **linux** - host OS; | |
| * **any-device-is-gpu** - device type to be available; | |
| * **any-device-is-level_zero** - backend to be available; | |
| * `linux` - host OS; | |
| * `any-device-is-gpu` - device type to be available; | |
| * `any-device-is-level_zero` - backend to be available; |
libsycl/test_e2e/README.md
Outdated
| Following features can be checked in tests to limit test execution to the | ||
| specific environment via REQUIRES, UNSUPPORTED, etc. filters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Following features can be checked in tests to limit test execution to the | |
| specific environment via REQUIRES, UNSUPPORTED, etc. filters. | |
| Following features can be passed to LIT via `REQUIRES`, `UNSUPPORTED`, etc. filters. | |
| to limit test execution to the specific environment. |
|
Let's drop "E2E" and just call these tests "libsycl tests" as all other runtime projects do. Pull request title: "[SYCL] E2E LIT tests initial configs & test" -> "[libsycl] Add lit configuration files and basic test" Pull request description:
|
| add_subdirectory(tools) | ||
|
|
||
| if(LLVM_INCLUDE_TESTS) | ||
| add_subdirectory(test_e2e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| add_subdirectory(test_e2e) | |
| add_subdirectory(test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left e2e in the name intentionally. In the end we will have same test folders as other projects: test, unittests + test_e2e. As in intel/llvm unittests and test won't require backend runtimes to be present on system to do check. while e2e testing require and so can't be joined with test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view, we have two top level test directories artificially created due to attempting to move some tests requiring third-party software/hardware to llvm-test-suite repository.
I see no reason to complicate the implementation with this separation. I don't see such complications in other runtime library projects depending on third-party software/hardware (e.g. liboffload, libc++, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have different configs for them. lit config and features for e2e tests are way more complex that is needed for test. test require compilation + sometimes tools like objdump to be run. That's it. I lot of stuff related to device choice, runtimes, build-only/run-only has no sense for that kind of tests.
I would prefer 2 different setups doing exactly what is needed for each kind of tests than merge them and have a monstrous config that should handle everything.
plus it may be easier to avoid a mess when compile-only and e2e tests are separated. we can have ./test/compile-only and ./test/e2e if you want to keep it under 'test' top folder but I don't see why it is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's simpler. I don't understand why we need to maintain to different LIT config files when "compile-only" tests are sub-set of regular tests. We can use single config for both types of tests.
libsycl/test_e2e/CMakeLists.txt
Outdated
| @@ -0,0 +1,36 @@ | |||
| cmake_minimum_required(VERSION 3.20.0) | |||
|
|
|||
| message("Configuring SYCL End-to-End Tests") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| message("Configuring SYCL End-to-End Tests") | |
| message("Configuring libsycl tests") |
Please, change "SYCL" to "libsycl" across the whole patch when we talk about the runtime library and drop end-to-end when refer to libsycl tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SYCl -> libsycl - ok, agree. But why should we drop end-to-end? I am adding e2e tests for now, but there are more tests coming (next phases) - unittests and tests that don't require offloading backends (abi, compile-only). I think we should differentiate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should differentiate them.
Why?
| config.libsycl_compiler = lit_config.params.get("libsycl_compiler", "@LIBSYCL_CXX_COMPILER@") | ||
| config.libsycl_root_dir= os.path.dirname(os.path.dirname(config.libsycl_compiler)) | ||
| config.libsycl_bin_dir = os.path.join(config.libsycl_root_dir, 'bin') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the code in CMake files I see that LIBSYCL_CXX_COMPILER points to the clang++ executiable inside the <compiler-root-dir>/bin, so I think this configuration will configure libsycl_bin_dir to <compiler-root-dir>/bin/bin. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No:
with
print (config.libsycl_compiler)
print (config.libsycl_root_dir)
print (config.libsycl_bin_dir)
in lit.cfg.py I got:
<some dir>/kt_llvm_project/build/bin/clang++
<some dir>/kt_llvm_project/build
<some dir>/kt_llvm_project/build/bin
l9: config.libsycl_root_dir= os.path.dirname(os.path.dirname(config.libsycl_compiler))
dirname is called twice and goes 2 levels up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to code it like this:
config.libsycl_compiler = lit_config.params.get("libsycl_compiler", "@LIBSYCL_CXX_COMPILER@")
config.libsycl_bin_dir = os.path.dirname(config.libsycl_compiler)
config.libsycl_root_dir = os.path.dirname(config.libsycl_bin_dir)?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ffc6ff4 to
f2d669c
Compare
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
f2d669c to
ebd8ced
Compare
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
c3b1e37 to
02df274
Compare
This PR brings initial version of lit configs for libsycl tests.
We plan to add more tests for libsycl integration with the compiler in the future.