[SYCL] Implementation of fallback assert #3767
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
March 31, 2021 17:07
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
|
@vzakhari , @jinge90 , could you, please, review devicelib changes? @dm-vodopyanov , @romanovvlad , could you, please, review DPCPP RT changes? |
vzakhari
previously approved these changes
Aug 27, 2021
added 2 commits
August 30, 2021 14:51
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
|
I couldn't reproduce the spec-consts caching regression locally. Hence, intel/llvm-test-suite#427. |
Can you explain more about the rationale here? It looks like there is a test that fails because of the new assertion fall-back code, and our solution is to disable assertion checks in that test? Doesn't this failing test point to some problem that needs to be fixed? |
Disabling of fallback assert in the test is temporary and failure is under active investigation at the moment. |
smaslov-intel
approved these changes
Aug 30, 2021
romanovvlad
reviewed
Aug 31, 2021
|
Failure on CUDA isn't related with the changes as it's observed during pull-down also: #4388 |
romanovvlad
approved these changes
Aug 31, 2021
gmlueck
approved these changes
Aug 31, 2021
bader
approved these changes
Aug 31, 2021
| @@ -6,19 +6,19 @@ int main() { | |||
| sycl::queue Q; | |||
|
|
|||
| Q.single_task([] { | |||
| // CHECK: tail call spir_func void @_Z21__spirv_MemoryBarrierjj(i32 2, i32 896) #2 | |||
| // CHECK: tail call spir_func void @_Z21__spirv_MemoryBarrierjj(i32 2, i32 896) #{{.*}} | |||
There was a problem hiding this comment.
Suggested change
| // CHECK: tail call spir_func void @_Z21__spirv_MemoryBarrierjj(i32 2, i32 896) #{{.*}} | |
| // CHECK: tail call spir_func void @_Z21__spirv_MemoryBarrierjj(i32 2, i32 896) |
vzakhari
approved these changes
Aug 31, 2021
dm-vodopyanov
approved these changes
Aug 31, 2021
jinge90
approved these changes
Sep 1, 2021
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jan 29, 2024
intel#6837 enabled asynchronous buffer destruction for buffers constructed without host data. However, initial fallback assert implementation in intel#3767 predates it and as such had to place the buffer inside `queue_impl` to avoid unintended synchronization point. I don't know if there was the same crash observed on the end-to-end test added as part of this PR prior to intel#3767, but it doesn't even matter because the "new" implementation is both simpler and doesn't result in a crash. I suspect that without it (with the buffer for fallback assert implementation being a data member of `sycl::queue_impl`) we had a cyclic dependency somewhere leading to resource leak and ultimately to the assert in `DeviceGlobalUSMMem::~DeviceGlobalUSMMem()`.
aelovikov-intel
added a commit
that referenced
this pull request
Jan 31, 2024
#6837 enabled asynchronous buffer destruction for buffers constructed without host data. However, initial fallback assert implementation in #3767 predates it and as such had to place the buffer inside `queue_impl` to avoid unintended synchronization point. I don't know if there was the same crash observed on the end-to-end test added as part of this PR prior to #3767, but it doesn't even matter because the "new" implementation is both simpler and doesn't result in a crash. I suspect that without it (with the buffer for fallback assert implementation being a data member of `sycl::queue_impl`) we had a cyclic dependency somewhere leading to resource leak and ultimately to the assert in `DeviceGlobalUSMMem::~DeviceGlobalUSMMem()`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Review documentation in #3461
Dependencies:
Regressions are workarounded/fixed in intel/llvm-test-suite#332, intel/llvm-test-suite#357, intel/llvm-test-suite#360
Exceptions report by host-task is included here from #4087