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.

Sign up for GitHub

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

Jump to bottom

Allow templated functors in parallel_for, parallel_reduce and parallel_scan #5976

Merged
merged 8 commits into from
May 10, 2023
Merged

Allow templated functors in parallel_for, parallel_reduce and parallel_scan #5976

merged 8 commits into from
May 10, 2023

Conversation

masterleinad
Copy link
Contributor

@masterleinad masterleinad commented Mar 9, 2023 •
edited

Part of #5908. Fixes #5156, improves #2034. This is basically #5420:

Extending FunctorAnalysis to be able to be used with templated call operators appears to be very difficult. On the other hand, the overloads of parallel_reduce and parallel_scan taking a return value already contain a hint for the value_type to use so that we can avoid using the deduction in FunctorAnalysis in those cases.

This pull request does exactly that. If the functor has a value_type alias, we use that, otherwise, we use the value_type deduced from the return type and if that is void, too, we do the deduction in FunctorAnalysis based on the functor's call operator.

edit: Copied description from #5420.

@masterleinad masterleinad mentioned this pull request Mar 9, 2023
Closed
@masterleinad masterleinad force-pushed the allow_generic_lambdas branch 4 times, most recently from 00cd644 to 817ad61 Compare March 10, 2023 18:04
masterleinad
masterleinad commented Mar 10, 2023
View reviewed changes
core/src/Kokkos_Parallel.hpp
Comment on lines +402 to +403
Impl::ParallelScanWithTotal<FunctorType, ExecutionPolicy,
typename ReturnType::value_type>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes a bug where we were passing the View as template argument instead of the value type.

@masterleinad masterleinad marked this pull request as ready for review March 10, 2023 18:15
@dalg24
Copy link
Member

dalg24 commented Mar 28, 2023

Please resolve the conflicts and improve the description

@masterleinad masterleinad mentioned this pull request Mar 28, 2023
Closed
@masterleinad
Copy link
Contributor Author

Please resolve the conflicts and improve the description

Done.

@masterleinad masterleinad mentioned this pull request Apr 5, 2023
Closed
@masterleinad masterleinad added this to the Release 4.1 milestone Apr 14, 2023
@masterleinad masterleinad added Enhancement Improve existing capability; will potentially require voting CHANGELOG Item to be included in release CHANGELOG labels Apr 14, 2023
@masterleinad masterleinad mentioned this pull request May 1, 2023
Merged
tcclevenger
tcclevenger reviewed May 4, 2023
View reviewed changes
core/unit_test/TestFunctorAnalysis.hpp
@@ -85,7 +85,7 @@ void test_functor_analysis() {
TestFunctorAnalysis_03 c03;
using A03 = Kokkos::Impl::FunctorAnalysis<
Kokkos::Impl::FunctorPatternInterface::REDUCE,
Kokkos::RangePolicy<ExecSpace>, TestFunctorAnalysis_03>;
Kokkos::RangePolicy<ExecSpace>, TestFunctorAnalysis_03, void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should a non-void OverrideValueType test be added where we static_assert the value type is the override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I added another test.

@Rombur
Copy link
Member

Rombur commented May 4, 2023

clang-format is not happy

@masterleinad
Copy link
Contributor Author

clang-format is not happy

Fixed.

@masterleinad
Copy link
Contributor Author

Only HIP-ROCm-5.2-C++20 is failing (it seems the machine was broken).

@dalg24
Copy link
Member

dalg24 commented May 9, 2023 •
edited

I don't like the complexity with the "lambda factory" capturing everything by reference but others seems to think it is fine.
Obviously not the right PR. Please ignore.

dalg24
dalg24 reviewed May 9, 2023
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Just posting pending comments that I had. Not a full review.

core/unit_test/incremental/Test05_ParallelReduce_RangePolicy.hpp Outdated Show resolved Hide resolved
core/unit_test/incremental/Test16_ParallelScan.hpp Outdated Show resolved Hide resolved
core/unit_test/incremental/Test16_ParallelScan.hpp Outdated Show resolved Hide resolved
core/unit_test/incremental/Test16_ParallelScan.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_FunctorAnalysis.hpp Show resolved Hide resolved
dalg24
dalg24 reviewed May 9, 2023
View reviewed changes
core/unit_test/incremental/Test16_ParallelScan.hpp Outdated
KOKKOS_FUNCTION void operator()(SizeType i) const {
d_data(i) = i * 0.5;
}
using View_1D = typename Kokkos::View<value_type *, ExecSpace>;
Copy link
Member

Choose a reason for hiding this comment

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

typename is superfluous here

Copy link
Member

Choose a reason for hiding this comment

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

Fixing is optional. Will merge if the CI passes.

@masterleinad
Copy link
Contributor Author

HIP CI didn't run. Everything else passes.

@dalg24 dalg24 merged commit c62a42e into kokkos:develop May 10, 2023
27 of 28 checks passed
@ndellingwood ndellingwood mentioned this pull request May 11, 2023
Closed
@masterleinad masterleinad mentioned this pull request May 25, 2023
Closed
nliber pushed a commit to nliber/kokkos that referenced this pull request Jun 22, 2023
@masterleinad @nliber
Allow templated functors in parallel_for, parallel_reduce and paralle…
9eaddfc 
…l_scan (kokkos#5976)

* Allow templated functors in parallel_for, parallel_reduce and parallel_scan

* Reorder template arguments for cuda_single_inter_block_reduce_scan_shmem

* Add another test to TestFunctorAnalysis.hpp

* Document OverrrideValueType some more

* Document that reducer functor is templated on purpose

* GenericScanFunctor->GenericExclusiveScanFunctor

* SizeType->IndexType

* Revert unnecessary changes in Test16_ParallelScan.hpp
@masterleinad masterleinad mentioned this pull request Nov 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 left review comments

@Rombur Rombur Rombur approved these changes

@nliber nliber nliber approved these changes

@tcclevenger tcclevenger tcclevenger approved these changes

Assignees
No one assigned
Labels
CHANGELOG Item to be included in release CHANGELOG Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Milestone
Release 4.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@masterleinad @dalg24 @Rombur @nliber @tcclevenger