Skip to content

[SYCL] Fix weak_object and owner_less for device objects #8740

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

Merged

Conversation

steffenlarsen
Copy link
Contributor

This commit fixes an issue where weak_object and in turn owner_less would fail to construct for SYCL object types that are usable on device. This was due to these object types only having an impl on host. As a result the kernel compilation would fail to get the impl of these. Since weak_object isn't intended for use inside kernels, this was fixed by using a dummy weak_ptr during kernel compilation to make it act like it is containing a weak pointer.

Previously this was not found in unittests because they are not compiled with the SYCL compiler. To avoid this breaking in the future, these tests are moved to the test-suite.

This commit fixes an issue where weak_object and in turn owner_less
would fail to construct for SYCL object types that are usable on device.
This was due to these object types only having an impl on host. As a
result the kernel compilation would fail to get the impl of these. Since
weak_object isn't intended for use inside kernels, this was fixed by
using a dummy weak_ptr during kernel compilation to make it act like
it is containing a weak pointer.

Previously this was not found in unittests because they are not compiled
with the SYCL compiler. To avoid this breaking in the future, these
tests are moved to the test-suite.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen requested a review from a team as a code owner March 22, 2023 19:10
@steffenlarsen
Copy link
Contributor Author

/verify with intel/llvm-test-suite#1678

@steffenlarsen steffenlarsen temporarily deployed to aws March 22, 2023 19:37 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws March 22, 2023 20:21 — with GitHub Actions Inactive
@AlexeySachkov AlexeySachkov requested a review from a team March 24, 2023 21:49
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen temporarily deployed to aws March 28, 2023 13:43 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws March 28, 2023 14:50 — with GitHub Actions Inactive
@steffenlarsen
Copy link
Contributor Author

@intel/llvm-reviewers-runtime | @sergey-semenov - Friendly ping.

Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall.
Although, do you think it would make sense to keep the unit tests and instead add a smaller device compilation only test for device objects to keep device compilation to a minimum in these tests? Or would there be too much code duplication between the two?

@steffenlarsen
Copy link
Contributor Author

LGTM overall. Although, do you think it would make sense to keep the unit tests and instead add a smaller device compilation only test for device objects to keep device compilation to a minimum in these tests? Or would there be too much code duplication between the two?

I worry that we would run the risk of once again ignore device-side issues by splitting it up. Most of the failures were either due to some invalid behavior during device-side compilation or due to the unittest host-compiler behaving differently than our compiler. Furthermore, a certain version of MSVC is having problem with the implementation, which manifests if they are used in the unittests but shouldn't manifest in e2e unless we use MSVC as the host compiler.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen temporarily deployed to aws March 30, 2023 20:58 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws March 30, 2023 23:21 — with GitHub Actions Inactive
@steffenlarsen
Copy link
Contributor Author

Failed Tests (1):
SYCL :: AtomicRef/atomic_memory_order_acq_rel.cpp
This test has since been disabled.

@steffenlarsen steffenlarsen merged commit b9b8513 into intel:sycl Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants