[REVIEW] improve device atomic overloads for potential issues #1691
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
split the file into `device_atomics.cuh` and `device_operators.cuh` separated the difinition of the device operators
move atomicCASImpl(int8 or int16) into typesAtomicCASImpl
simplify atomicMin, atomixMax add cudf::bool8 for atomic test case for atomicAdd,Min,Max add cudf::bool8 specialization for genericAtomicOperation
jrhemstad
requested changes
May 9, 2019
kovaltan
changed the title
kovaltan
added
3 - Ready for Review
harrism
requested changes
May 13, 2019
Add '__forceinline__ __device__' to `W genericAtomicOperator(W)`
Add size check assert between `long long int` and `int64_t`
remove redundant `sizeof(T)` when calling 'typesAtomicCASImpl`
remove redundant `sizeof(T)` when calling 'genericAtomicOperationImpl`
|
rerun tests |
|
rerun tests |
jrhemstad
requested changes
May 14, 2019
Add native atomicAdd(uint64_t) call for sint64_t
harrism
approved these changes
May 14, 2019
Add comment for `genericAtomicOperationImpl<int64_t, DeviceSum, 8>` why it uses atomicAdd(uint64) inside
Removed `genericAtomicOperation(W)` since it is not invoked for cudf::wrapper types. Merged it into `genericAtomicOperation(T)` Add size check assert at `type_reinterpret`.
jrhemstad
approved these changes
May 14, 2019
cpp/src/utilities/device_atomics.cuh
Outdated
| } | ||
|
|
||
| // specialization for cudf::detail::wrapper types | ||
| template <typename T, gdf_dtype dtype, typename BinaryOp, typename W = cudf::detail::wrapper<T, dtype> > |
There was a problem hiding this comment.
This shouldn't be removed. I rely on it in #1478
It will need to be added back in a future PR.
jrhemstad
force-pushed
the
bug_atomic_update
branch
from
9a3a178 to
edc14cb
Compare
|
Fixed by #1735 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update/improve device atomic overloads for potential issues, and simplify the implementation.
Then, it would be easy to implement when cudf changes the underlying type of the wrappers,
or cudf introduces a new data type.
And this PR also introduce cuda native
atomicAddsupport for signed int64_t. I didn't implemented it yet since cuda does not havesigned long long intoverload, however,two's complementrepresentation of signed integer has the advantage that the fundamental arithmetic operations of addition, subtraction, and multiplication are identical to those for unsigned binary numbers. See also: https://en.wikipedia.org/wiki/Two%27s_complementdevice_operators.cuhgenericAtomicOperationUnderlyingTypetypesAtomicOperation32|64atomicAddsupport for signed int64_tCloses #1398
Related #1685