Skip to content

[SYCLomatic]Remove reverse_iterator from partition implementations#2350

Merged
zhimingwang36 merged 1 commit intooneapi-src:SYCLomaticfrom
danhoeflinger:dev/dhoeflin/remove_reverse_iter
Sep 11, 2024
Merged

[SYCLomatic]Remove reverse_iterator from partition implementations#2350
zhimingwang36 merged 1 commit intooneapi-src:SYCLomaticfrom
danhoeflinger:dev/dhoeflin/remove_reverse_iter

Conversation

@danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Sep 10, 2024

Partition implementations should not use std::reverse_iterator, because this disallows usage of sycl buffer iterator-like inputs returned from oneapi::dpl::[begin,end]. MSVC STL runs into compiler errors for reverse_iterator of this iterator-like type.

Without a reverse iterator, we no longer can gain advantage from providing a non-stable partition implementation, so we go back to having a unified implementation (these were separated in #2215). If in the future oneDPL provides a reverse_iterator which is compatible with sycl buffer iterator-like types, we can improve this implementation by removing this extra copy call, and extra tmp data allocation. However, for now I think this is the best we can do.

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
@danhoeflinger danhoeflinger requested a review from a team as a code owner September 10, 2024 15:01
Copy link
Contributor

@timmiesmith timmiesmith left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM. We may want to create an issue somewhere about this problem if we want to consider adding a reverse iterator to oneDPL in the future.

@danhoeflinger
Copy link
Contributor Author

LGTM. We may want to create an issue somewhere about this problem if we want to consider adding a reverse iterator to oneDPL in the future.

uxlfoundation/oneDPL#1836

Copy link
Contributor

@tomflinda tomflinda left a comment

Choose a reason for hiding this comment

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

LGTM

@zhimingwang36 zhimingwang36 merged commit 347f89d into oneapi-src:SYCLomatic Sep 11, 2024
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.

5 participants