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

[HOTFIX] Overload for atomicAdd on int64 #1735

Merged
merged 19 commits into from
May 14, 2019

Conversation

jrhemstad
Copy link
Contributor

Supersedes #1691

Because #1691 was based off of branch-0.8, I had to cherry-pick it's commits onto a new branch based off of release-0.7.2.

In order to hotfix the abysmal performance of groupby when summing int64, provides an overload for atomicAdd int64 that simply casts to uint64. Because 2s complement is used, this is safe.

Compared to other systems for representing signed numbers (e.g., ones' complement), two's complement has the advantage that the fundamental arithmetic operations of addition, subtraction, and multiplication are identical to those for unsigned binary numbers (as long as the inputs are represented in the same number of bits - as the output, and any overflow beyond those bits is discarded from the result). This property makes the system simpler to implement, especially for higher-precision arithmetic.

https://en.wikipedia.org/wiki/Two%27s_complement

kovaltan added 16 commits May 14, 2019 08:36
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
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`
Add native atomicAdd(uint64_t) call for sint64_t
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 jrhemstad requested a review from a team as a code owner May 14, 2019 15:42
@jrhemstad jrhemstad added libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond labels May 14, 2019
CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Keith Kraus <keith.j.kraus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Review Waiting for reviewer to review or respond libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants