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 radix test failure involving integer sign #561

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

brian-kelley
Copy link
Contributor

@brian-kelley brian-kelley commented Jan 11, 2020

Now, the serial radix sorts enforce at compile time that the key is an unsigned integer.
This was really the assumption all along (that values were always
nonnegative). No longer adding a bias to the keys while sorting, since all the keys are at least 0.

This was causing the char radix sort unit test to fail in an OpenMP debug configuration - I think because of undefined overflow behavior when the chars were normalized from range (min, max) to range (0, max-min). This PR sorts only nonnegative chars and reinterprets as unsigned char, which is safe.

Now, serial radix sorts enforce that the key is unsigned.
This was really the assumption all along (that values were always
nonnegative).
@brian-kelley brian-kelley self-assigned this Jan 11, 2020
@brian-kelley
Copy link
Contributor Author

Bowman PR: blas trsm tests failing due to #559, unrelated to these changes

#######################################################
PASSED TESTS
#######################################################
intel-16.4.258-Pthread-release build_time=726 run_time=1038
intel-16.4.258-Pthread_Serial-release build_time=1043 run_time=1992
intel-16.4.258-Serial-release build_time=696 run_time=942
intel-17.2.174-OpenMP-release build_time=883 run_time=561
intel-17.2.174-OpenMP_Serial-release build_time=1210 run_time=1481
intel-17.2.174-Pthread-release build_time=811 run_time=922
intel-17.2.174-Pthread_Serial-release build_time=1164 run_time=1806
intel-17.2.174-Serial-release build_time=775 run_time=900
#######################################################
FAILED TESTS
#######################################################
intel-18.2.199-OpenMP-release (test failed)
intel-18.2.199-OpenMP_Serial-release (test failed)
intel-18.2.199-Pthread-release (test failed)
intel-18.2.199-Pthread_Serial-release (test failed)
intel-18.2.199-Serial-release (test failed)

RIDE PR: failures are due to #344 (also unrelated)

#######################################################
PASSED TESTS
#######################################################
cuda-9.2.88-Cuda_OpenMP-release build_time=576 run_time=417
cuda-9.2.88-Cuda_Serial-release build_time=491 run_time=525
gcc-6.4.0-OpenMP_Serial-release build_time=204 run_time=395
gcc-7.2.0-OpenMP-release build_time=126 run_time=129
gcc-7.2.0-OpenMP_Serial-release build_time=216 run_time=361
gcc-7.2.0-Serial-release build_time=120 run_time=226
ibm-16.1.0-Serial-release build_time=494 run_time=398
#######################################################
FAILED TESTS
#######################################################
ibm-16.1.0-OpenMP-release (test failed)
ibm-16.1.0-OpenMP_Serial-release (test failed)

Copy link
Contributor

@srajama1 srajama1 left a comment

Choose a reason for hiding this comment

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

Thanks @brian-kelley !

@srajama1
Copy link
Contributor

Can we add documentation for sorting routines in the internal repo ?

@brian-kelley brian-kelley merged commit 96cf575 into kokkos:develop Jan 13, 2020
@ndellingwood
Copy link
Contributor

@brian-kelley did this get tested with Tpetra? I'm assuming this may impact spadd where we previously ran into an issue during promotion testing a couple weeks back?

@brian-kelley
Copy link
Contributor Author

@ndellingwood Hmm, I didn't try it with Tpetra. But it doesn't call the sort functions directly so it should be fine. Spadd is currently the only place where these sorts are called, but I would like to update Tpetra::Details::sortCrsEntries to use them.

This was referenced May 20, 2024
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.

3 participants