This repository was archived by the owner on Mar 28, 2023. It is now read-only.
[SYCL][HIP] Fix assert tests #1083
Merged
Conversation
This file contains hidden or 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
Assert is now implemented for HIP. Additionally fixing the assert multiple tus tests led to fixing the `sycl-external.cpp` test. It turns out that the AMDGPU llvm backend was running some dead code elimination and deleting `SYCL_EXTERNAL` functions, this is also why `kernel_bundle_ignore_sycl_external.cpp` worked on AMD, not because it was behaving correctly but because the DCE was deleting the `SYCL_EXTERNAL` function so it looked "ignored" from the kernel bundle, this test is now marked as failing again.
|
That's a good point, but I don't think it would be helpful because in this case it hits an The tests worked locally but they might end up being flaky as well, and since it's all in the driver I'm not sure there's much we can do but leave them marked unsupported again. |
|
Changes look good to me. Thanks. But I will hold off on approval till there is some analysis/comment about the new test failures. |
bader
pushed a commit
to intel/llvm
that referenced
this pull request
Aug 2, 2022
This patch adds an implementation of `__assert_fail` for HIP. It is implemented in IR because the function needs to take pointers with no address space and OpenCL compilation makes them private. It is implemented using the OCKL printf functions provided by HIP. In addition this patch also fixes `SYCL_EXTERNAL` for HIP, the AMDGPU backend runs some dead code elimination and would simply delete `SYCL_EXTERNAL` functions, and so this patch prevents the AMDGPU backend from deleting `SYCL_EXTERNAL` functions, identifying them through the `sycl-module-id` attribute (as done in other places such as `sycl-post-link`). This issue showed up in some of the assert tests that use multiple translation units, and this patch also fixes the `sycl-external.cpp` test. Matching `llvm-test-suite` update: intel/llvm-test-suite#1083
myler
pushed a commit
to myler/llvm-test-suite
that referenced
this pull request
Mar 22, 2023
Assert is now implemented for HIP. Additionally fixing the assert multiple tus tests led to fixing the `sycl-external.cpp` test. It turns out that the AMDGPU llvm backend was running some dead code elimination and deleting `SYCL_EXTERNAL` functions, this is also why `kernel_bundle_ignore_sycl_external.cpp` worked on AMD, not because it was behaving correctly but because the DCE was deleting the `SYCL_EXTERNAL` function so it looked "ignored" from the kernel bundle, this test is now marked as failing again.
aelovikov-intel
pushed a commit
to aelovikov-intel/llvm
that referenced
this pull request
Mar 27, 2023
Assert is now implemented for HIP. Additionally fixing the assert multiple tus tests led to fixing the `sycl-external.cpp` test. It turns out that the AMDGPU llvm backend was running some dead code elimination and deleting `SYCL_EXTERNAL` functions, this is also why `kernel_bundle_ignore_sycl_external.cpp` worked on AMD, not because it was behaving correctly but because the DCE was deleting the `SYCL_EXTERNAL` function so it looked "ignored" from the kernel bundle, this test is now marked as failing again.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Assert is now implemented for HIP.
Additionally fixing the assert multiple tus tests led to fixing the
sycl-external.cpptest. It turns out that the AMDGPU llvm backend wasrunning some dead code elimination and deleting
SYCL_EXTERNALfunctions, this is also why
kernel_bundle_ignore_sycl_external.cppworked on AMD, not because it was behaving correctly but because the DCE
was deleting the
SYCL_EXTERNALfunction so it looked "ignored" fromthe kernel bundle, this test is now marked as failing again.
The tests in this PR are fixed by intel/llvm#6424