[SYCL] Implement SYCL2020 reductions with set initialize_to_identity … #3410
Conversation
v-klochkov
force-pushed
the
public_vklochkov_reduction_2020_discard_write
branch
from
de9293a to
0c254dd
Compare
…property The corresponding LIT tests PR: intel/llvm-test-suite#194 SYCL2020 reductions use dynamic information (instead of static info used previously) to ask to do discard-write to user's reduction variable or USM memory. That caused big changes removing/re-structuring the code that previously relied on accessor-mode (read-write vs discard-write). With this patch the SYCL-2020 reductions become on par with ONEAPI::reduction Signed-off-by: Vyacheslav N Klochkov <[email protected]>
v-klochkov
force-pushed
the
public_vklochkov_reduction_2020_discard_write
branch
from
0c254dd to
8d3af03
Compare
|
Will try to review on Monday |
Signed-off-by: Vyacheslav N Klochkov <[email protected]>
…reduction_2020_discard_write
| if (MRWAcc) | ||
| CGH.associateWithHandler(MRWAcc.get(), access::target::global_buffer); | ||
| if (MDWAcc) | ||
| CGH.associateWithHandler(MDWAcc.get(), access::target::global_buffer); |
There was a problem hiding this comment.
Please, add assert that only one accessor is defined.
There was a problem hiding this comment.
My initial move was to add the assert here:
assert((!MRWAcc || !MDWAcc) && "Reduction cannot be created with read-write AND discard-write accessor");
But then I thought that MRWAcc and MDWAcc fields are initialized only in reduction_impl constructor and those places are obvious, thus having that assert here would be pretty much useless/redundant.
There was a problem hiding this comment.
I going though to change the line 'if (MDWAcc)' to 'else if (MDWAcc)'
| shared_ptr_class<rw_accessor_type>{}, &Acc)), | ||
| MIdentity(Identity), MBinaryOp(BOp), InitializeToIdentity(false) { | ||
| if (Acc.get_count() != 1) | ||
| throw runtime_error("Reduction variable must be a scalar.", |
There was a problem hiding this comment.
Could you please clarify what scalar means in this context?
There was a problem hiding this comment.
I used the word 'scalar' here as opposite to 'vector'. Accessor may be 1-dimensional, but it must not be a true 1-dimensional array, it may have only 1 element, i.e. be a scalar.
| DataLessPropKindSize = 7, | ||
| InitializeToIdentity = 8 | ||
| InitializeToIdentity = 7, | ||
| DataLessPropKindSize = 8 |
There was a problem hiding this comment.
It seems changing DataLessPropKindSize can be an ABI break since it can change MDataLessProps.
But, it looks like the minimum value already allocated for MDataLessProps is 4 bytes, so that should be OK.
Can we define something like DataLessPropKindMax = 32 with comment "exceeding this is an ABI break" and use it as the size of MDataLessProps? Could you please do it as a separate PR?
There was a problem hiding this comment.
Yes, sure, I'll do that in a separate PR.
There was a problem hiding this comment.
Created that PR here: #3458
Signed-off-by: Vyacheslav N Klochkov <[email protected]>
…reduction_2020_discard_write
|
The only failed test Jenkins/Precommit is SYCL :: Sampler/unnormalized-clamp-linear-float.cpp. It is unrelated to this PR. |
…reduction_2020_discard_write
…y property The corresponding changes in SYCL RT: intel/llvm#3410 intel/llvm#3481 Signed-off-by: Vyacheslav N Klochkov <[email protected]>
* [SYCL] Add test cases for SYCL2020 reductions + initialize_to_identity property The corresponding changes in SYCL RT: intel/llvm#3410 intel/llvm#3481 This patch - adds checks for SYCL-2020 reductions with initialize_to_identity property (#3410) - enables the test reduction_nd_N_vars.cpp for level-zero (#3481) - reduces some of tests by eliminating unneeded/duplicated checks. - removes the test reduction_transparent.cpp as useless because transparent operators already checked in many other tests. - reduces runtime of reduction tests by about 200x in average by re-using queue created once instead of creating it in each of test-cases Signed-off-by: Vyacheslav N Klochkov <[email protected]>
) * [SYCL] Add test cases for SYCL2020 reductions + initialize_to_identity property The corresponding changes in SYCL RT: intel#3410 intel#3481 This patch - adds checks for SYCL-2020 reductions with initialize_to_identity property (intel/llvm-test-suite#3410) - enables the test reduction_nd_N_vars.cpp for level-zero (intel/llvm-test-suite#3481) - reduces some of tests by eliminating unneeded/duplicated checks. - removes the test reduction_transparent.cpp as useless because transparent operators already checked in many other tests. - reduces runtime of reduction tests by about 200x in average by re-using queue created once instead of creating it in each of test-cases Signed-off-by: Vyacheslav N Klochkov <[email protected]>
…property
The corresponding LIT tests PR: intel/llvm-test-suite#194
SYCL2020 reductions use dynamic information (instead of static info used
previously) to ask to do discard-write to user's reduction variable or USM
memory. That caused big changes removing/re-structuring the code that
previously relied on accessor-mode (read-write vs discard-write).
With this patch the SYCL-2020 reductions become on par with ONEAPI::reduction
Signed-off-by: Vyacheslav N Klochkov [email protected]