Repository navigation
Add env overloads for DeviceSegmentedRadixSort - #7999
Conversation
2c8d80a to
55ca027
Compare
This comment has been minimized.
This comment has been minimized.
NaderAlAwar
left a comment
NaderAlAwar
left a comment
There was a problem hiding this comment.
Important: we are missing the DoubleBuffer overloads
@NaderAlAwar will be added in subsequent pr for economy of reviewing effort per pr |
This comment has been minimized.
This comment has been minimized.
Only because our radix-sort is stable by design. Unstable sort could be non-deterministic 😉 |
| //! yield a corresponding performance improvement. | ||
| //! - Note, the size of any segment may not exceed ``INT_MAX``. Please consider using ``DeviceSegmentedSort`` instead, | ||
| //! if the size of at least one of your segments could exceed ``INT_MAX``. | ||
| //! |
There was a problem hiding this comment.
Maybe we should have a @env like the @devicestorage on the old APIs to avoid the repetition. Even if it needs multiple like @env-with-guarantees.
There was a problem hiding this comment.
I considered suggesting the same! No need to do it in this PR though.
There was a problem hiding this comment.
this has already been suggested on a friday review meeting and it in my todo list. i will open an issue
| //! - An optional bit subrange ``[begin_bit, end_bit)`` of differentiating key | ||
| //! bits can be specified. This can reduce overall sorting overhead and | ||
| //! yield a corresponding performance improvement. | ||
| //! - Note, the size of any segment may not exceed ``INT_MAX``. Please consider using ``DeviceSegmentedSort`` instead, |
There was a problem hiding this comment.
Missing
//! - Let ``in`` be one of ``{d_keys_in, d_values_in}`` and `out` be any of
//! ``{d_keys_out, d_values_out}``. The range ``[out, out + num_items)`` shall
//! not overlap ``[in, in + num_items)``,
//! ``[d_begin_offsets, d_begin_offsets + num_segments)`` nor
//! ``[d_end_offsets, d_end_offsets + num_segments)`` in any way.
and
//! - Segments are not required to be contiguous. For all index values ``i``
//! outside the specified segments ``d_keys_in[i]``, ``d_values_in[i]``,
//! ``d_keys_out[i]``, ``d_values_out[i]`` will not be accessed nor modified.
Maybe also
//! - @devicestorageNP For sorting using only ``O(P)`` temporary storage, see
//! the sorting interface using DoubleBuffer wrappers below.
| } | ||
|
|
||
| //! @rst | ||
| //! Overview |
There was a problem hiding this comment.
This "title" is new in comparison to the existing docs. If it stays, it looks like it should be underlined (rendered as a sub-title)
| //! Overview | |
| //! Overview | |
| //! +++++++++++++++++++++++++++++++++++++++++++++ |
Same for Snippet below (which is not new but has this underline in existing docs)
| //! - An optional bit subrange ``[begin_bit, end_bit)`` of differentiating key | ||
| //! bits can be specified. This can reduce overall sorting overhead and | ||
| //! yield a corresponding performance improvement. | ||
| //! - Note, the size of any segment may not exceed ``INT_MAX``. Please consider using ``DeviceSegmentedSort`` instead, |
| REQUIRE(keys_out == expected_keys); | ||
| } | ||
|
|
||
| TEST_CASE("DeviceSegmentedRadixSort::SortPairs uses custom stream", "[segmented_radix_sort][device]") |
There was a problem hiding this comment.
Should this be tested for all overloads?
bernhardmgruber
left a comment
bernhardmgruber
left a comment
There was a problem hiding this comment.
Please apply the remaining feedback from other reviewers. Otherwise LGTM
c577169 to
a2838b4
Compare
🥳 CI Workflow Results🟩 Finished in 1h 11m: Pass: 100%/249 | Total: 3d 19h | Max: 53m 01s | Hits: 94%/157462See results here. |
* Add env overloads for DeviceSegmentedRadixSort * Simplify literalinclude paths * Fix docs * Remove _CCCL_FORCEINLINE * fix docs * Fix static_casts and types in api test * Add uses custom stream for all algorithms
fixes #7549
No deterministic guarantees. It's sorting, if it's not deterministic it's just wrong.