Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Stable sorting order for -0.0 and +0.0 for float and double. #218

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

canonizer
Copy link
Contributor

This modifies digit extraction in radix sorting to sort -0.0 and +0.0 in stable order.

@alliepiper alliepiper added this to the 1.11.1 milestone Oct 19, 2020
@alliepiper
Copy link
Collaborator

@canonizer Have you looked into at the performance impact of this? If not, I can use this as a case study for my new benchmarking code, but that will probably mean this won't make the 1.11.0 release.

@alliepiper alliepiper self-assigned this Oct 27, 2020
@alliepiper alliepiper modified the milestones: 1.11.1, 1.11.0 Oct 27, 2020
@alliepiper
Copy link
Collaborator

alliepiper commented Oct 27, 2020

This implementation looks good to me, and the performance difference is negligible (<1% difference on GV100/GA100 from @canonizer's benchmarks).

Though now I'm curious whether this is actually expected behavior for users, or if it's better to just let them pre-condition their data when they want these to be treated equivalently. I'll do some polls and confirm whether or not this is something folks expect.

@alliepiper
Copy link
Collaborator

alliepiper commented Oct 27, 2020

Looks like the stl already does stable sorts this way, so that's a nice, definitive answer :)

@canonizer Can you update the DeviceRadixSort documentation to specify this behavior, and add a regression test to test/test_device_radix_sort.cu? I'll take a look at the other backends in Thrust to see if we can provide this guarantee there, too.

@canonizer
Copy link
Contributor Author

I will update the documentation and add the test.

@alliepiper alliepiper assigned canonizer and unassigned alliepiper Oct 30, 2020
@alliepiper
Copy link
Collaborator

@canonizer Any updates on this? I'd need to land this by Friday to make the 1.11.0 release, otherwise we can slip to 1.12.0.

@brycelelbach brycelelbach modified the milestones: 1.11.0, 1.12.0 Nov 16, 2020
@alliepiper alliepiper dismissed their stale review December 9, 2020 19:44

Changes addressed.

@alliepiper
Copy link
Collaborator

This looks good to me, I'll start testing soon. Thanks Andy!

@alliepiper alliepiper added the testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). label Dec 9, 2020
@alliepiper
Copy link
Collaborator

Heads up -- I rebased and squashed this to make this work with our scripts. We don't handle merging main into topic branches very well.

@alliepiper
Copy link
Collaborator

DVS CL: 29407401

@alliepiper alliepiper added the testing: gpuCI in progress Started gpuCI testing. label Dec 9, 2020
@alliepiper alliepiper assigned alliepiper and unassigned canonizer Dec 10, 2020
@alliepiper alliepiper added testing: gpuCI passed Passed gpuCI testing. testing: internal ci passed Passed internal NVIDIA CI (DVS). and removed testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Dec 10, 2020
@alliepiper alliepiper merged commit 6d51715 into NVIDIA:main Jan 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
testing: gpuCI passed Passed gpuCI testing. testing: internal ci passed Passed internal NVIDIA CI (DVS).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants