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

[oneDPL] Fixing zip_iterator for value_type and base() return #605

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danhoeflinger
Copy link
Contributor

The existing zip_iterator class specification uses std::tuple as the value_type and also the return of base().
This is problematic for device copyability for dpcpp device backend and is not how oneDPL has implemented zip_iterator for quite some time. std::tuple is not required to be trivially copyable, which causes pain when dealing with zip_iterator and SYCL kernels, requiring is_device_copyable specializations for any types which are composed of zip_iterator::value_type.

We currently leave pointer as std::tuple (it is still a std::tuple in our implementation), but could consider changing it in this PR to be more broad in case of future changes. pointer seems less of an issue from a device copyable standpoint, but it still may be nice to use a consistent unspecified tuple across the type, and changing it here would give more freedom in the future.

I view this as a bugfix for the specification, rather than a breaking change, but that is of course up for discussion.

@danhoeflinger
Copy link
Contributor Author

oneDPL has made significant progress in adding SYCL device copyable specializations for oneDPL's iterator types and wrapper functions. There was a question about whether this removes the need for a trivially copyable internal tuple for reference, value types, and the base iterator. I don't think it does.

I've reviewed why it's important for zip_iterator to have a trivially copyable value_type for trivially copyable base iterator value types. Some SYCL group helpers used in oneDPL, like group_broadcast and shift_group_right, require trivially copyable types. These are essential for the best performing scan algorithms. This wasn't the original reason for oneDPL's trivially copyable tuple, but it supports keeping it in zip_iterator.

I believe the original motivation was to support device copyability of types without adding to issues which arise when types contain device copyable but not trivially copyable members. Recent work on oneDPL's iterators and internal wrapper types has alleviated some of this within oneDPL. However, moving back to std::tuple as value or reference type would burden users who use the those types as a member field within a callable or other type. For use in sycl kernels (via oneDPL or otherwise) they would need to create a SYCL device copyable specialization, where now that do not have to do that. We can avoid adding to this problem by maintaining our trivially copyable tuple as a reference and value type.

@rarutyun may better describe the original motivation, but I'll continue working to get some concrete cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant