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

OpenMPTarget init-join fix #6444

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Conversation

rgayatri23
Copy link
Contributor

The PR adds verification for OpenMP process and initialization in init-join reducers.
This also fixes the backward_compatibility test failure with the backend.

@rgayatri23 rgayatri23 self-assigned this Sep 13, 2023
@rgayatri23 rgayatri23 added Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) Backend - OpenMPTarget Kokkos-Core labels Sep 13, 2023
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.

How is calling two functions that check preconditions fixing anything?

@dalg24
Copy link
Member

dalg24 commented Sep 13, 2023

If the issue is related to setting the value of MAX_ACTIVE_THREADS

OpenMPTargetExec::MAX_ACTIVE_THREADS =
Kokkos::Experimental::OpenMPTarget().concurrency();

why is initialization in OpenMPTargetExec::verify_initialized the proper place to do that?
Why can't it be done at initialization of the backend?

@rgayatri23
Copy link
Contributor Author

If the issue is related to setting the value of MAX_ACTIVE_THREADS

OpenMPTargetExec::MAX_ACTIVE_THREADS =
Kokkos::Experimental::OpenMPTarget().concurrency();

why is initialization in OpenMPTargetExec::verify_initialized the proper place to do that?
Why can't it be done at initialization of the backend?

Yes the issue is related to MAX_ACTIVE_THREADS. That variable is used to calculate maximum in flight teams which in-turn determines the scratch space allocated.
The variable can be set during the backend initialization, which is probably where it should go I guess if we only want to do it only once.

@masterleinad
Copy link
Contributor

The variable can be set during the backend initialization, which is probably where it should go I guess if we only want to do it only once.

Would there be a reason to do it more than once?

@rgayatri23
Copy link
Contributor Author

The variable can be set during the backend initialization, which is probably where it should go I guess if we only want to do it only once.

Would there be a reason to do it more than once?

No since the arch wont change in between the kernel invocations. I moved the call into initialize.

Comment on lines 200 to 203
OpenMPTargetExec::verify_is_process(
"Kokkos::Experimental::OpenMPTarget parallel_for");
OpenMPTargetExec::verify_initialized(
"Kokkos::Experimental::OpenMPTarget parallel_for");
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firstly, the message label was wrong which I fixed now.
Secondly, I think yes , we are checking for those in other similar routines too. Not having here was an oversight.

Comment on lines 86 to 89
OpenMPTargetExec::verify_is_process(
"Kokkos::Experimental::OpenMPTarget parallel_for");
"Kokkos::Experimental::OpenMPTarget parallel_reduce");
OpenMPTargetExec::verify_initialized(
"Kokkos::Experimental::OpenMPTarget parallel_for");
"Kokkos::Experimental::OpenMPTarget parallel_reduce");
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to call these functions in execute directly before the dispatch to execute_init_join, execute_reducer,...? In that case, you could also be more precise about the policy used in the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edited the error message.

Comment on lines 118 to 119
Kokkos::Impl::OpenMPTargetExec::MAX_ACTIVE_THREADS =
Kokkos::Experimental::OpenMPTarget().concurrency();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Kokkos::Impl::OpenMPTargetExec::MAX_ACTIVE_THREADS =
Kokkos::Experimental::OpenMPTarget().concurrency();
Kokkos::Impl::OpenMPTargetExec::MAX_ACTIVE_THREADS =
concurrency();

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.

OK with me.

@dalg24
Copy link
Member

dalg24 commented Sep 19, 2023

The openmptarget build passed. Not waiting on the rest

@dalg24
Copy link
Member

dalg24 commented Sep 19, 2023

@rgayatri23 make sure you add to the changelog
When was this bug introduced? Did you investigate?

@dalg24 dalg24 merged commit 7e27496 into kokkos:develop Sep 19, 2023
27 of 28 checks passed
@rgayatri23
Copy link
Contributor Author

@rgayatri23 make sure you add to the changelog When was this bug introduced? Did you investigate?

Yeah this was introduced in when we did #6043

@fnrizzi fnrizzi mentioned this pull request Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend - OpenMPTarget Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) Kokkos-Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants