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

Enable {transform_}exclusive_scan in place #6667

Merged
merged 7 commits into from
Jan 5, 2024

Conversation

aprokop
Copy link
Collaborator

@aprokop aprokop commented Dec 13, 2023

currently Kokkos exclusive_scan does not work for in-place operation.
The c++ std says that exclusive_scan can support inplace, so this PR fixes the defect.
This PR fixes that and adds new tests.
While adding new tests, we also improve things to reduce duplication, use proper gtest << operator if needed, removing useless comments and adding new explanations.

use case

In ArborX, it is typical to do some some counting into a View, and then do a scan on that view to convert it to offsets. Creating a temporary extra view comes at the cost of additional memory and performance.
I don't think C++ standard says anything about whether it's allowable for src and dst to be the same.

@dalg24
Copy link
Member

dalg24 commented Dec 13, 2023

Please include a code snippet that shows what usage this enables.
We will need to add tests for them.

@dalg24
Copy link
Member

dalg24 commented Dec 13, 2023

I don't think C++ standard says anything about whether it's allowable for src and dst to be the same.

The standard explicitly says that they may be equal. https://eel.is/c++draft/numeric.ops#exclusive.scan-8

Meaning if we don't currently support it, this can be considered as a defect report.

@aprokop
Copy link
Collaborator Author

aprokop commented Dec 13, 2023

Please include a code snippet that shows what usage this enables.
We will need to add tests for them.

From ArborX perspective, a simple example when we want to preallocate storage for queries for k-nearest neighbor kernels, where each predicate can be asking for a different number of neighbors:

auto const n = queries.size();
Kokkos::View<int*, MemorySpace> offsets(Kokkos::view_alloc(
    space, "offsets", Kokkos::WithoutInitializing), n + 1);
Kokkos::parallel_for("scan_queries",
    Kokkos::RangePolicy<ExecutionSpace>(space, 0, n),
    KOKKOS_LAMBDA(int i) { offsets(i) = queries(i).k; });
Kokkos::exclusive_scan(space, offsets, offsets);

@aprokop
Copy link
Collaborator Author

aprokop commented Dec 13, 2023

@fnrizzi What testing would you want to see here?

@aprokop
Copy link
Collaborator Author

aprokop commented Dec 13, 2023

The standard explicitly says that they may be equal. eel.is/c++draft/numeric.ops#exclusive.scan-8

The same page says the same holds true for the inclusive_scan. I haven't checked whether Kokkos' inclusive_scan also suffers from the same issue. At the minimum, would make sense to check and to add tests there.

@fnrizzi
Copy link
Contributor

fnrizzi commented Dec 18, 2023

we can modify the existing test to ensure the source and dest are the same , and also fix the inclusive_scan accordingly.
@aprokop let me know if you can do that, or if you want me to do this

It is interesting that while the std draft explicitly has a remark about this, the std doc does not say anything.
https://en.cppreference.com/w/cpp/algorithm/exclusive_scan

@aprokop
Copy link
Collaborator Author

aprokop commented Dec 18, 2023

@fnrizzi please go ahead

@dalg24
Copy link
Member

dalg24 commented Dec 18, 2023

It is interesting that while the std draft explicitly has a remark about this, the std doc does not say anything. https://en.cppreference.com/w/cpp/algorithm/exclusive_scan

In the parameters description

d_first - the beginning of the destination range; may be equal to first

@fnrizzi
Copy link
Contributor

fnrizzi commented Dec 18, 2023

Ah good eye, I totally missed it

@fnrizzi fnrizzi marked this pull request as draft December 19, 2023 16:18
@fnrizzi fnrizzi changed the title Enable exclusive_scan in place Enable exclusive_scan in place Dec 20, 2023
@fnrizzi fnrizzi marked this pull request as ready for review December 20, 2023 08:09
@fnrizzi fnrizzi changed the title Enable exclusive_scan in place Enable {transform_}exclusive_scan in place Dec 20, 2023
@fnrizzi fnrizzi marked this pull request as draft December 20, 2023 14:53
@fnrizzi fnrizzi marked this pull request as ready for review December 20, 2023 18:59
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

@dalg24 dalg24 merged commit 3523bc3 into kokkos:develop Jan 5, 2024
29 checks passed
@dalg24 dalg24 mentioned this pull request Jan 5, 2024
@dalg24
Copy link
Member

dalg24 commented Jan 5, 2024

@aprokop did you only do exclusive scan because that's all you needed or because inclusive scan does not have the defect?

@fnrizzi
Copy link
Contributor

fnrizzi commented Jan 5, 2024

@dalg24 there is a PR I opened for it where I am adding the test. Inclusive scan works

@dalg24
Copy link
Member

dalg24 commented Jan 5, 2024

Cool. I was just checking https://godbolt.org/z/e4M4xnncY

@aprokop aprokop deleted the exclusive_scan_in_place branch January 5, 2024 14:36
@fnrizzi
Copy link
Contributor

fnrizzi commented Jan 5, 2024

Cool. I was just checking https://godbolt.org/z/e4M4xnncY

yes, seems to work, i still have to finalize PR will do that soon

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.

None yet

4 participants