Support for SYCL#230
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines. |
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
Realized a bit late of the pending tests for JIT, AoT modes of Kokkos build with EKAT in debug/release modes. Will update the PR to ready-to-review as soon as possible when all the checks are complete. |
…on just in case of a need mostly for SYCL backend
abagusetty
added
the
SYCL
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
5 similar comments
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Hi @bartgol, just checking to see if I have the right labels after removing the WIP to proceed for review. |
bartgol
left a comment
bartgol
left a comment
There was a problem hiding this comment.
Thanks! Looks nice (and relatively small, considered that we are adding support for a new backend)!
I have a couple of comments. Nothing major, but wanted to hear your thoughts.
| */ | ||
| template <bool Serialize, typename TeamMember, typename Lambda, typename ValueType> | ||
| #ifdef KOKKOS_ENABLE_SYCL | ||
| KOKKOS_INLINE_FUNCTION SYCL_EXTERNAL |
There was a problem hiding this comment.
We do have lots of KOKKOS_INLINE_FUNCTION in EKAT. Why does this particular case require a special handling? And what does this macro do differently from KOKKOS_INLINE_FUNCTION?
There was a problem hiding this comment.
There is a bit of limitation on how static device-side functions and linking can be setup with SYCL. There are majorly two changes suggested for this function alone and has nothing to do with KOKKOS_INLINE_FUNCTION.
Regarding the need of SYCL_EXTERNAL: In the case of linking C++ functions to a SYCL application, where the definitions are not available in the same translation unit of the compiler, then the macro SYCL_EXTERNAL has to be provided. More info here
I have made some changes to cleanup this. Thanks.
There was a problem hiding this comment.
Ah, I misread the two macros as just a single one.
There was a problem hiding this comment.
But still, why do we need external here? The parallel_reduce fcn is defined inline here, so it should be available in all translation units. Am I missing something?
There was a problem hiding this comment.
That is exactly right. The definition available to all other translation units during linking. Mostly a temporary one, this will be improved in the future revisions of the specifications and the compiler implementations.
There was a problem hiding this comment.
But then we should not need SYCL_EXTERNAL at all, right?
There was a problem hiding this comment.
Making the function static was being problematic to device-side kernel. SYCL_EXTERNAL attr and removing static was suggested. I have slightly modified the function signature to look as below, please let me know if that looks okay
template <bool Serialize, typename TeamMember, typename Lambda, typename ValueType>
#ifdef KOKKOS_ENABLE_SYCL
SYCL_EXTERNAL
#else
static
#endif
KOKKOS_INLINE_FUNCTION
void parallel_reduce (const TeamMember& team,
const int& begin, // pack index
const int& end, // pack index
const Lambda& lambda,
ValueType& result)There was a problem hiding this comment.
Are you saying that SYCL complains about the function being static, and requires the external keyword to compile? Either I am not understanding SYCL, or this seems wrong. The "inline" and "extern" keywords are somewhat mutually exclusive, so I am surprised that SYCL needs that macro. The documentation suggests that it is needed if the definition is not available to other TU's, but since it's inlined, it should be available as soon as this header is included.
Anyhow, if you say that it does not compile without that macro, then it's fine. I'm just not really happy with this confusing detail.
There was a problem hiding this comment.
Sorry for the confusion. The need for SYCL_EXTERNAL is not required any more. There has been a mix-up with compiler versions at my end. Thanks for pointing this out.
|
|
||
| # Link MPI | ||
| target_link_libraries (ekat PUBLIC MPI::MPI_C) | ||
| if (Kokkos_ROOT) |
There was a problem hiding this comment.
This if statement is not necessary if we do the mod to CMakeLists.txt suggested in the comment above.
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; This inspection will remain valid until a new commit to source branch is performed. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Blake
Jenkins Parameters
Using Repos:
Pull Request Author: abagusetty |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Blake
Jenkins Parameters
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Mappy # 317 (click to expand)Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Weaver # 414 (click to expand)Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Blake # 431 (click to expand) |
|
I checked the actual log of the jenkins job, and it appears the wsm test is failing with CUDA, with the following error(s): |
Fixed CUDA/HIP/SYCL test failure
|
For V100 abagusetty@gpu02 /gpfs/jlse-fs0/projects/climate/abagusetty/EKAT_sycl/build_cuda_v100 (feature/sycl) $ ctest
Test project /gpfs/jlse-fs0/projects/climate/abagusetty/EKAT_sycl/build_cuda_v100
Start 1: lin_interp_omp1
1/30 Test #1: lin_interp_omp1 .................. Passed 1.10 sec
Start 2: tridiag
2/30 Test #2: tridiag .......................... Passed 11.09 sec
Start 3: tridiag_invalid_flags
3/30 Test #3: tridiag_invalid_flags ............ Passed 0.34 sec
Start 4: array_io
4/30 Test #4: array_io ......................... Passed 0.54 sec
Start 5: yaml_parser
5/30 Test #5: yaml_parser ...................... Passed 0.52 sec
Start 6: kokkos_utils_omp1
6/30 Test #6: kokkos_utils_omp1 ................ Passed 0.56 sec
Start 7: wsm_omp1
7/30 Test #7: wsm_omp1 ......................... Passed 0.62 sec
Start 8: kernel_on_host
8/30 Test #8: kernel_on_host ................... Passed 0.53 sec
Start 9: comm_np1
9/30 Test #9: comm_np1 ......................... Passed 0.52 sec
Start 10: comm_np2
10/30 Test #10: comm_np2 ......................... Passed 0.63 sec
Start 11: comm_np3
11/30 Test #11: comm_np3 ......................... Passed 0.73 sec
Start 12: comm_np4
12/30 Test #12: comm_np4 ......................... Passed 0.85 sec
Start 13: packs
13/30 Test #13: packs ............................ Passed 0.56 sec
Start 14: pack_utils
14/30 Test #14: pack_utils ....................... Passed 0.52 sec
Start 15: units
15/30 Test #15: units ............................ Passed 0.52 sec
Start 16: debug_tools
16/30 Test #16: debug_tools ...................... Passed 0.52 sec
Start 17: meta_utils
17/30 Test #17: meta_utils ....................... Passed 0.55 sec
Start 18: util_cxx
18/30 Test #18: util_cxx ......................... Passed 0.51 sec
Start 19: string_utils
19/30 Test #19: string_utils ..................... Passed 0.52 sec
Start 20: upper_bound
20/30 Test #20: upper_bound ...................... Passed 0.64 sec
Start 21: factory
21/30 Test #21: factory .......................... Passed 0.52 sec
Start 22: math_util
22/30 Test #22: math_util ........................ Passed 0.52 sec
Start 23: regress_fail
23/30 Test #23: regress_fail ..................... Passed 0.51 sec
Start 24: catch_main_invalid_flags
24/30 Test #24: catch_main_invalid_flags ......... Passed 0.20 sec
Start 25: serial_file_log
25/30 Test #25: serial_file_log .................. Passed 0.52 sec
Start 26: mpi_file_log_tests_np1
26/30 Test #26: mpi_file_log_tests_np1 ........... Passed 0.53 sec
Start 27: mpi_file_log_tests_np2
27/30 Test #27: mpi_file_log_tests_np2 ........... Passed 0.63 sec
Start 28: mpi_file_log_tests_np3
28/30 Test #28: mpi_file_log_tests_np3 ........... Passed 0.74 sec
Start 29: mpi_file_log_tests_np4
29/30 Test #29: mpi_file_log_tests_np4 ........... Passed 0.85 sec
Start 30: console_only_log_np4
30/30 Test #30: console_only_log_np4 ............. Passed 0.88 sec
100% tests passed, 0 tests failed out of 30
Label Time Summary:
MustFail = 1.05 sec*proc (3 tests)
Total Test time (real) = 28.31 sec |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; This inspection will remain valid until a new commit to source branch is performed. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Blake
Jenkins Parameters
Using Repos:
Pull Request Author: abagusetty |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Blake
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ bartgol ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
|
Merge on Pull Request# 230: IS A SUCCESS - Pull Request successfully merged |
|
Congrats on merging your first pull request! We hope this is only the first of many to come. |
3 participants
SYCL&Kokkos::Experimental::SYCLhas certain limitations, those are attended via compiler defined macro__SYCL_DEVICE_ONLY__