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

Use oneapi::dpl::sort_by_key if possible #928

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

masterleinad
Copy link
Collaborator

Fixes #926. I didn't see much of a difference in performance but that also wasn't really expected at this point.

Comment on lines 175 to 176
#if ONEDPL_VERSION_MAJOR > 2022 || \
(ONEDPL_VERSION_MAJOR == 2022 && ONEDPL_VESION_MINOR >= 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just require this? What's the minimum version for Kokkos? Do we care?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kokkos doesn't have a minimum version; as of now, we are only using dpl::sort anyway.
oneDPL is not necessarily recent enough on the testbeds so I wouldn't want to require 2022.2 at this point.

@masterleinad masterleinad marked this pull request as ready for review August 16, 2023 13:07
@aprokop
Copy link
Contributor

aprokop commented Aug 16, 2023

@masterleinad please confirm that you verified that the code executes the sort by key with the new onedpl. In other words, that the macro check is correct.

Co-authored-by: Damien L-G <dalg24+github@gmail.com>
@masterleinad
Copy link
Collaborator Author

@masterleinad please confirm that you verified that the code executes the sort by key with the new onedpl. In other words, that the macro check is correct.

I tested without the macro guards before on the testbeds (with newer than default compiler versions, i.e., an oneDPL commit that includes sort_by_key based on 2022.1.1), and that worked fine.

@masterleinad
Copy link
Collaborator Author

I also tested manually with the 2022.2.0 oneDPL release which works fine. However, we get a bunch more warnings and it's not clear to me if it's beneficial to update the CI (here or in a follow-up pull request).

Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

LGTM

@aprokop aprokop merged commit 25e20e6 into arborx:master Aug 16, 2023
1 check passed
@aprokop aprokop deleted the use_dpl_sort_by_key branch December 12, 2023 23:28
@aprokop aprokop added the performance Something is slower than it should be label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Something is slower than it should be
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OneDPL added sort_by_key
3 participants