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

Fix device-side conversions to 64-bit integer types. #1205

Merged
merged 1 commit into from
Aug 27, 2019

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Aug 26, 2019

Signed-off-by: Michal Zientkiewicz michalz@nvidia.com

Why we need this PR?

  • It fixes a bug caused by long and long long overload resolution problem
  • It works around a bug in CUDA/compiler when converting FP to 64-bit integers in round to nearest mode

What happened in this PR?

* Add inline to rounding helper functions.
* Work around a bug in __float2ll_rn and related functions.
* Add tests for device-side conversions to 64-bit integers.
* Fix `dev_to_string(long long)` for input value of -2^63.

JIRA TASK: [DALI-1031]

@mzient
Copy link
Contributor Author

mzient commented Aug 26, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [873349]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [873349]: BUILD FAILED

@mzient
Copy link
Contributor Author

mzient commented Aug 26, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [873548]: BUILD STARTED

@mzient
Copy link
Contributor Author

mzient commented Aug 26, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [873580]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [873548]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [873580]: BUILD PASSED

dali/core/convert_cuda_test.cu Show resolved Hide resolved
return __double2int_rn(f);
}
__device__ unsigned cuda_round_helper(double f, unsigned) { // NOLINT
inline __device__ unsigned cuda_round_helper(double f, unsigned) { // NOLINT
return __double2uint_rn(f);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to add a comment explaining the reason behind the workaround, for future readers of this file

@@ -139,6 +141,9 @@ inline __device__ DeviceString dev_to_string(long long x) { // NOLINT
if (x < 0) {
neg = true;
x = -x;
if (x < 0) {
return "-9223372036854775808";
Copy link
Contributor

Choose a reason for hiding this comment

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

don't really understand this

Copy link
Contributor Author

@mzient mzient Aug 27, 2019

Choose a reason for hiding this comment

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

That's decimal representation of -2^63. U2 numbers are asymmetric and n-bit signed number cannot represent +2^(n-1), but can represent -2^(n-1). Negating -(-2^(n-1)) == -2^n(-1) due to overflow.
I could add a special case for this particular number.

Work around a bug in __float2ll_rn and related functions.
Add tests for device-side conversions to 64-bit integers.
Fix `dev_to_string(long long)` for input value of -2^63.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Copy link
Contributor Author

mzient commented Aug 27, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [875079]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [875079]: BUILD PASSED

@mzient mzient merged commit 6bbc76d into NVIDIA:master Aug 27, 2019
@mzient mzient deleted the FixGPUConvert branch August 27, 2019 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants