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

Added multiple reducers support for team-level parallel reduce #5727

Merged
merged 19 commits into from
May 5, 2023

Conversation

ldh4
Copy link
Contributor

@ldh4 ldh4 commented Jan 4, 2023

Resolves #4510

Added an interface to allow parallel_reduce with TeamThreadRange, ThreadVectorRange or TeamVectorRange policy to be called with more than one reducer.

@ldh4
Copy link
Contributor Author

ldh4 commented Jan 13, 2023

Retest this please.

1 similar comment
@ldh4
Copy link
Contributor Author

ldh4 commented Jan 16, 2023

Retest this please.

@ldh4 ldh4 marked this pull request as ready for review February 6, 2023 16:04
core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Combined_Reducer.hpp Outdated Show resolved Hide resolved
core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
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.

@ldh4 ldh4 requested a review from dalg24 April 27, 2023 19:55
core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
EXPECT_EQ(n, hostView(0));
EXPECT_EQ((n * (n + 1) / 2), hostView(1));
EXPECT_EQ(n * n * (n + 1) / 2, hostView(2));
EXPECT_EQ(n * n, hostView(3));
Copy link
Member

Choose a reason for hiding this comment

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

Why 4 combined reduction rather than 3 or 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason. Just wanted to test something more than just 2 reducers.

core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
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.

Please explicitly compare against reduction identity when the range is empty.
Looks good other than that.

core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
core/unit_test/TestTeamCombinedReducers.hpp Outdated Show resolved Hide resolved
@ldh4 ldh4 added this to the Release 4.1 milestone May 3, 2023
@dalg24
Copy link
Member

dalg24 commented May 3, 2023

I plan on ignoring

5: [ RUN      ] cuda.workgraph_fib
5: /var/jenkins/workspace/Kokkos/core/unit_test/TestWorkGraph.hpp:127: Failure
5: Expected equality of these values:
5:   h_values(0)
5:     Which is: 5778
5:   full_fibonacci(m_input)
5:     Which is: 6765
5: [  FAILED  ] cuda.workgraph_fib (1426 ms)

in the CUDA-11.6-NVCC build. Still waiting on at least one of the HIP build to pass before I merge.

@ldh4
Copy link
Contributor Author

ldh4 commented May 4, 2023

Retest this please.

@dalg24 dalg24 merged commit e247508 into kokkos:develop May 5, 2023
28 checks passed
@dalg24 dalg24 mentioned this pull request May 5, 2023
nliber pushed a commit to nliber/kokkos that referenced this pull request Jun 22, 2023
…s#5727)

* Added interfaces and unit-tests for combined reducers supports for
TeamThreadRange, ThreadVectorRange, TeamVectorRange

* fixed warnings from unit tests

* Fixed errors from CI tests

* Added n=0 test cases

* fixed warnings from unit tests

* Removed team combined reducers unit-test file from OpenACC and OpenMPTarget tests list

* quick syntax fix

* Adjusted unit test skip conditions for openmptarget and openacc

* Put in a macro guard to check if KOKKOS_ENABLE_CUDA_LAMBDA is defined

* Addressing comments from reviews

* Update core/src/impl/Kokkos_Combined_Reducer.hpp

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

* Update core/unit_test/TestTeamCombinedReducers.hpp

Co-authored-by: Damien L-G <dalg24+github@gmail.com>

* Converted write_one_value_back_on_device to a static function

* Clang-formatted

* git rebasing

* Removed unnecessary fences from parallel_reduce_impl

* Adjusted unit tests based on feedbacks

* Adjusted expect_eq values in the unit tests

* Removed a few ternary conditions

---------

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
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

5 participants