Skip to content
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

Host allocation sharing via set final data - construction->destruction #502

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

mkinsner
Copy link

I'm not completely sure of this fix, but I think "destruction" fits better here since set_final_data is called post-construction. Maybe better to say "valid at buffer destruction" instead of "after", but that matters less.

@mkinsner
Copy link
Author

The existing wording seems to reflect one very specific use case of set_final_data, where an initial host allocation which was bound to the buffer becomes unavailable and needs to be replaced with an alternate host allocation, during the buffer's lifetime. There are however other use cases for set_final_data, and the pointer set through that API only matters across the buffer destruction boundary AFAIK.

@gmlueck
Copy link
Contributor

gmlueck commented Nov 27, 2023

Note that the paragraph you changed will be removed in #298 along with the entire section that contains it. Change #298 is currently pending agreement on the exact semantics of set_final_data.

I'm not really opposed to the change you propose, but I think we need a bigger clarification to this part of the spec.

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Let's commit this anyway since even if it is to be replaced later, it is still an improvement today.

@gmlueck
Copy link
Contributor

gmlueck commented Dec 12, 2023

This PR needs one more reviewer.

@gmlueck
Copy link
Contributor

gmlueck commented Dec 14, 2023

We could use one more reviewer here before today's WG meeting.

Copy link
Collaborator

@AerialMantis AerialMantis left a comment

Choose a reason for hiding this comment

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

Good catch, looks good.

@gmlueck
Copy link
Contributor

gmlueck commented Dec 14, 2023

WG approved

@gmlueck gmlueck merged commit cd5768d into SYCL-2020/master Dec 14, 2023
3 checks passed
keryell pushed a commit that referenced this pull request Sep 10, 2024
Host allocation sharing via set final data - construction->destruction
gmlueck added a commit that referenced this pull request Nov 7, 2024
Host allocation sharing via set final data - construction->destruction

(cherry picked from commit cd5768d)
gmlueck added a commit that referenced this pull request Nov 7, 2024
Host allocation sharing via set final data - construction->destruction

(cherry picked from commit cd5768d)
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.

4 participants