Skip to content

[SYCL] Make internal device_global pointer mutable #8780

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
merged 3 commits into from
Mar 28, 2023

Conversation

steffenlarsen
Copy link
Contributor

The implementation of device_global allows it to be const. However, for cases where the device_global is shared between programs, the value of the device_global is defined through a USM pointer inside the device_global. Since this pointer is seen as const when the device_global is const-qualified, the compiler may chose to optimize it out, potentially causing invalid accesses on device. To avoid this, this commit marks the underlying pointer member as mutable which prevents this potential optimization.

The implementation of device_global allows it to be const. However, for
cases where the device_global is shared between programs, the value of
the device_global is defined through a USM pointer inside the
device_global. Since this pointer is seen as const when the
device_global is const-qualified, the compiler may chose to optimize it
out, potentially causing invalid accesses on device. To avoid this, this
commit marks the underlying pointer member as mutable which prevents
this potential optimization.

Signed-off-by: Larsen, Steffen <[email protected]>
@steffenlarsen steffenlarsen requested a review from a team as a code owner March 27, 2023 14:27
@steffenlarsen steffenlarsen force-pushed the steffen/mutable_dg_ptr branch from a8c70a8 to 7847203 Compare March 27, 2023 14:27
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

@@ -44,7 +44,7 @@ template <typename T, typename PropertyListT, typename = void>
class device_global_base {
protected:
using pointer_t = typename decorated_global_ptr<T>::pointer;
pointer_t usmptr{};
mutable pointer_t usmptr{};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to have a comment here that briefly mentions why this has to be mutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! It has been added.

@steffenlarsen steffenlarsen temporarily deployed to aws March 27, 2023 15:06 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws March 27, 2023 16:27 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws March 28, 2023 13:31 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws March 28, 2023 14:24 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen merged commit cb9486d into intel:sycl Mar 28, 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