-
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
Try removing volatile from AtomicDataElement #5455
Try removing volatile from AtomicDataElement #5455
Conversation
operator value_type() const { | ||
return Kokkos::Impl::atomic_load(ptr, Kokkos::Impl::memory_order_relaxed); |
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.
This and operator=
might be the only significant changes.
Is that UniqueToken failure in OpenMPTarget a thing which occasionally goes wrong because of a pre-existing bug, or is there something real there changing due to this PR? |
We have seen that failing before. That test doesn't even use |
I think this is good. |
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'm happy with the code change.
It would be useful to get some statistics on the failing test, to see how often it failed randomly before this change, and whether that's increased with it
Ignoring the OpenMPTarget failure |
In the last hackathon I was mentoring, my team wanted to use
MemorytTraits:::Atomic
with a custom type. This currently requires defining all kinds ofvolatile
qualified member functions which is not very user-friendly.It appears that none of the
volatile
overloads inAtomicDataElement
are used anymore, though, and there doesn't seem to be a point in combingvolatile
withatomic
.Also,
Kokkos_Atomic_is_only_allowed_with_32bit_and_64bit_scalars
is not used at all.