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

Fixed sanitizer warnings in RadixSortScanBinsKernel #277

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

canonizer
Copy link
Contributor

@canonizer canonizer commented Mar 29, 2021

Fixed sanitizer warnings in RadixSortScanBinsKernel.

@canonizer canonizer changed the title Fixed sanitizer warnings in upsweep-downsweep sort Fixed sanitizer warnings in RadixSortScanBinsKernel Mar 29, 2021
@alliepiper alliepiper added the type: bug: functional Does not work as intended. label Mar 29, 2021
@alliepiper alliepiper added this to the 1.13.0 milestone Mar 29, 2021
@brycelelbach brycelelbach added the P1: should have Necessary, but not critical. label Mar 29, 2021
Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

Question about the implementation.

Once the patch is finalized we should do a quick check to see if this has any significant perf impact.

cub/device/dispatch/dispatch_radix_sort.cuh Outdated Show resolved Hide resolved
@alliepiper
Copy link
Collaborator

@canonizer I'd like to get this in sometime this week to land in 1.13. Should I just remove the dead code mentioned in my review? Is there anything else that this needs to be ready?

@alliepiper alliepiper self-requested a review June 11, 2021 14:30
alliepiper added a commit to alliepiper/thrust that referenced this pull request Jun 11, 2021
@alliepiper
Copy link
Collaborator

DVS CL: 30066471
gpuCI: NVIDIA/thrust#1460

- correct per-pass spine length
- correctly handling the last scan tile
alliepiper added a commit to alliepiper/thrust that referenced this pull request Jun 14, 2021
@alliepiper
Copy link
Collaborator

Fixed a new warning.

DVS CL: 30073602
gpuCI: NVIDIA/thrust#1460

@alliepiper alliepiper added testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Jun 14, 2021
@alliepiper alliepiper assigned alliepiper and unassigned canonizer Jun 15, 2021
@alliepiper alliepiper merged commit 866c576 into NVIDIA:main Jun 15, 2021
@jlebar
Copy link

jlebar commented Jun 15, 2021

Found this PR from the release notes for the 1.13 RC.

What is the practical effect of this change? Without it, we do OOB memory reads (and writes?), which might crash depending on page sizes?

@canonizer
Copy link
Contributor Author

canonizer commented Jun 15, 2021

I don't think there's any practical effect, apart from fixing the sanitizer warning.

There has been no out-of-bounds reads. Rather, uninitialized memory was fed into a scan, which caused it to be read, which triggered a warning. However, the results that were derived from uninitialized memory locations weren't used anywhere, so the original warning was benign.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1: should have Necessary, but not critical. testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). type: bug: functional Does not work as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants