-
Notifications
You must be signed in to change notification settings - Fork 407
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: UniqueToken::Global implementation. #3823
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not approving since I co-wrote this. Generally I am good with this implementation: note the instance test should work, its just the team level thing which doesn't. The failing test is the SYCL build which failes while compiling the LLVM??
core/unit_test/TestUniqueToken.hpp
Outdated
@@ -166,8 +166,11 @@ TEST(TEST_CATEGORY, unique_token_global) { | |||
} | |||
|
|||
TEST(TEST_CATEGORY, unique_token_instance) { | |||
// FIXME_OPENMPTARGET - Not yet implemented. | |||
#if !defined(KOKKOS_ENABLE_OPENMPTARGET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this works? Its just the acquire_team_unique_token which fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes right, enabled it back.
I have encountered this earlier. Restarting the test resolved the issue. |
UniqueToken& operator=(UniqueToken&&) = default; | ||
|
||
/// \brief upper bound for acquired values, i.e. 0 <= value < size() | ||
KOKKOS_INLINE_FUNCTION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KOKKOS_INLINE_FUNCTION | |
KOKKOS_FUNCTION |
same comment below
uint32_t volatile* m_buffer; | ||
uint32_t m_count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crtrott would you remind me why we need these? why can't we directly use m_buffer_view.data()
and m_buffer_view.size()
? Does it have to do with the volatile
-qualifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the global unique token was implemented that way from the beginning, and yes it is because of the volatile. I think the OpenMPTarget specialization doesn't necessarily need that, and we could just use a view here. Same for CUDA and the HIP. Maybe we should do this as a separate PR and change them uniformely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the OpenMP / Pthread ones need the volatile)
* OpenMPTarget: UniqueToken::Global implementation. * OpenMPTarget: Adding the UniqueToken.hpp file. * OpenMPTarget: Enable UniqueTokenScope::Instance unit test. * OpenMPTarget: Protecting memcpy in Kokkos_OpenMPTarget_Instance.cpp with `OMPT_SAFE_CALL` macro. * OpenMPTarget: Replace array with std::vector. * OpenMPTarget: Prefix `Kokkos` to UniqueToken buffer view label. Co-authored-by: rgayatri <rgayatri@lbl.gov>
The PR contains the following updates:
m_uniquetoken_ptr
to OpenMPTargetExec class.