radix sort support unsigned/signed type #1367
Conversation
Summary of ChangesHello @deadlywing, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the radix sort functionality by introducing explicit support for unsigned integer types. This ensures that sorting operations correctly handle the distinct characteristics of signed and unsigned numbers, particularly in descending order and when interpreting bit patterns. The changes involve adding type identification utilities, refining the core sorting algorithm, and bolstering test coverage to validate the new behavior across various integer types. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for unsigned integer types to the radix sort implementation. The changes include introducing a helper function isUInteger and a corresponding macro, updating the Value class, and modifying the radix sort logic in _gen_bv_vector to correctly handle both signed and unsigned types. New tests have been added to verify the correctness of sorting for unsigned types and the signed interpretation of unsigned values. The changes look good and the tests are comprehensive. I have one suggestion to simplify the logic for handling the most significant bit in permute.cc to improve readability.
There was a problem hiding this comment.
Pull request overview
This PR adds support for correctly handling both unsigned and signed integer types in radix sort operations. The implementation distinguishes between signed and unsigned types to properly handle the sign bit during bit decomposition for sorting.
Key changes:
- Added type utility functions to identify unsigned integer types
- Modified radix sort bit decomposition logic to handle sign bits differently for signed vs unsigned types
- Added comprehensive test coverage for unsigned types, signed type interpretation, and boolean keys
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libspu/core/type_util.h | Defines FOREACH_UINT_DTYPES macro and declares isUInteger function to identify unsigned integer types |
| src/libspu/core/type_util.cc | Implements isUInteger function using the new macro |
| src/libspu/core/value.h | Adds isUInt() method to Value class for checking unsigned integer types |
| src/libspu/kernel/hal/permute.cc | Updates radix sort bit decomposition to correctly handle sign bits for signed vs unsigned types |
| src/libspu/kernel/hal/permute.h | Adds documentation noting that users must ensure correct signed/unsigned type usage |
| src/libspu/kernel/hlo/sort_test.cc | Adds comprehensive tests for unsigned type sorting, signed interpretation, and boolean key sorting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // IMPORTANT: the user should ensure that the data has the correct signed or | ||
| // unsigned type. |
There was a problem hiding this comment.
The comment "IMPORTANT: the user should ensure that the data has the correct signed or unsigned type" would be more helpful if it explained the consequences of not doing so. Consider expanding it to clarify what happens if the data type is incorrect, such as: "IMPORTANT: the user should ensure that the data has the correct signed or unsigned type. Incorrect type interpretation will result in incorrect sort order (e.g., treating signed values as unsigned will place negative numbers at the end instead of the beginning)."
| // unsigned type. | |
| // unsigned type. Incorrect type interpretation will result in incorrect sort | |
| // order (for example, treating signed values as unsigned may place negative | |
| // numbers at the end instead of the beginning). |
| // | ||
| // Important notes: | ||
| // - for radix sort, the user should ensure that the data has the correct | ||
| // signed or unsigned type. |
There was a problem hiding this comment.
The documentation note "for radix sort, the user should ensure that the data has the correct signed or unsigned type" would be more helpful if it explained the consequences and provided guidance. Consider expanding it to: "for radix sort, the user should ensure that the data has the correct signed or unsigned type. The sort implementation uses the type information to correctly handle the sign bit. Using the wrong type (e.g., treating signed integers as unsigned) will result in incorrect sort order where negative values are sorted as large positive values."
| // signed or unsigned type. | |
| // signed or unsigned type. The sort implementation uses the type | |
| // information to correctly handle the sign bit. Using the wrong type | |
| // (for example, treating signed integers as unsigned) will result in | |
| // incorrect sort order where negative values are sorted as large | |
| // positive values. |
| xt::xarray<bool> sorted_k1_desc = {true, true, true, false, false}; | ||
| xt::xarray<bool> sorted_k1_asc = {false, false, true, true, true}; | ||
|
|
||
| // Expected payloads (sorted within each group since sort is unstable) |
There was a problem hiding this comment.
The comment "Expected payloads (sorted within each group since sort is unstable)" is misleading. The expected payload arrays on lines 619-620 and 622-623 are not pre-sorted within each group - they represent the expected order if the sort were stable (maintaining original relative order). The actual sorting within groups happens later in the test code (lines 649-654 and 684-689) to account for potential instability. Consider clarifying the comment to: "Expected payloads (assuming stable sort - will be sorted within groups for comparison to handle instability)".
| // Expected payloads (sorted within each group since sort is unstable) | |
| // Expected payloads (assuming stable sort - will be sorted within groups for comparison to handle instability) |
| #define FOREACH_UINT_DTYPES(FN) \ | ||
| FN(DT_I1, I1, 1) \ | ||
| FN(DT_U8, U8, 8) \ | ||
| FN(DT_U16, U16, 16) \ | ||
| FN(DT_U32, U32, 32) \ | ||
| FN(DT_U64, U64, 64) |
There was a problem hiding this comment.
The macro name FOREACH_UINT_DTYPES could be misleading since it includes DT_I1 (bool), which uses an "I" prefix typically associated with signed integers, not unsigned integers. While treating bool as unsigned for sorting purposes is semantically correct (false=0 < true=1), the macro name doesn't clearly convey this intent. Consider renaming to something like FOREACH_UNSIGNED_OR_BOOL_DTYPES or adding a comment explaining why DT_I1 is included in this macro.
Pull Request
What problem does this PR solve?
Issue Number: Fixed #
Possible side effects?
Performance:
Backward compatibility: