FastIntDiv: fallback to normal division when values exceed 32-bit ran… - #3093
Conversation
…ge during run time.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughChanges
FastIntDiv behavior and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/raft/util/fast_int_div.cuh`:
- Around line 102-105: Update the range guards in both fast integer division and
modulo operators to reject any operand above INT32_MAX, not merely values with
nonzero bits 63:32, and fall back to native division/modulo for all
overflow-risk inputs. Preserve the existing fast path only when both operands
are within the supported signed 32-bit range, and add regression vectors
covering UINT32_MAX, divisors above INT32_MAX, and matching native
quotient/remainder results.
In `@cpp/tests/util/fast_int_div.cu`:
- Around line 25-39: Update the numerator coverage in
CompareWithNativeDivisionInt32 and the corresponding int64 test to include
INT32_MIN and INT64_MIN explicitly. Preserve the existing magnitude/sign
combinations and divisor coverage while adding these signed minimum boundary
values directly to the tested numerator vectors.
- Around line 41-54: Extend the magnitudes and/or divisors in
CompareWithNativeDivisionInt64 with values immediately beyond the signed 32-bit
boundary, including INT32_MAX + 2 and UINT32_MAX. Keep the existing
signed-positive and negative numerator coverage, and ensure both operator/ and
operator% remain compared against native division and remainder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 15ea5333-6f28-4096-bea3-68f7254928d7
📒 Files selected for processing (2)
cpp/include/raft/util/fast_int_div.cuhcpp/tests/util/fast_int_div.cu
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp/include/raft/util/fast_int_div.cuh (3)
74-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInitialize scalar members on the fallback path.
A directly constructed unsupported
int64_tdivisor returns before assigningmandp. The copy constructor then reads those indeterminate members, even though later operations use the fallback path.Proposed fix
- UIntT m; + UIntT m{}; ... - int p; + int p{};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/raft/util/fast_int_div.cuh` around lines 74 - 90, Initialize the scalar members m and p before the unsupported-divisor early return in computeScalars, ensuring directly constructed int64_t divisors leave both members in a defined state while fallback remains enabled. Preserve the existing handling for valid, zero, and negative divisors.
78-90: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winClear stale fallback state before recomputing scalars.
After assigning a supported divisor to an instance that previously held an unsupported one,
fallbackremains true and permanently bypasses the fast path. Reset it at the start ofcomputeScalars()and add a fallback-to-supported assignment regression test.Proposed fix
void computeScalars() { + fallback = false; if (d == 1) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/raft/util/fast_int_div.cuh` around lines 78 - 90, Reset fallback to false at the beginning of computeScalars() before evaluating the divisor, so recomputation after changing from an unsupported divisor can re-enable the fast path. Add a regression test that assigns an unsupported divisor, then a supported divisor, and verifies fast-path scalar computation is used.
111-136: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConstrain the new overloads to integral numerators.
These templates now accept floating-point numerators. For example,
5.5f / FastIntDiv<int32_t>{2}selects this overload, truncates5.5fto5, and returns2.0frather than2.75f. Restrict both overloads to integralNumIntT.Proposed fix
-template <typename NumIntT, typename DivIntT> +template <typename NumIntT, + typename DivIntT, + std::enable_if_t<std::is_integral_v<NumIntT>, int> = 0> HDI std::common_type_t<NumIntT, DivIntT> operator/(NumIntT n, const FastIntDiv<DivIntT>& divisor) -template <typename NumIntT, typename DivIntT> +template <typename NumIntT, + typename DivIntT, + std::enable_if_t<std::is_integral_v<NumIntT>, int> = 0> HDI std::common_type_t<NumIntT, DivIntT> operator%(NumIntT n, const FastIntDiv<DivIntT>& divisor)As per path instructions,
cpp/REVIEW_GUIDELINES.mdrequires public operator template signature changes to be reviewed for downstream impact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/raft/util/fast_int_div.cuh` around lines 111 - 136, Restrict the public operator/ and operator% templates for FastIntDiv to integral NumIntT types using the project’s existing constraint or enablement convention. Preserve the current integer behavior and return types for supported numerators, while preventing floating-point numerators such as float from selecting either overload.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/raft/util/fast_int_div.cuh`:
- Around line 18-19: Add a direct <limits> include to the header containing
kInt32Min and kInt32Max so std::numeric_limits is explicitly declared without
relying on transitive includes.
---
Outside diff comments:
In `@cpp/include/raft/util/fast_int_div.cuh`:
- Around line 74-90: Initialize the scalar members m and p before the
unsupported-divisor early return in computeScalars, ensuring directly
constructed int64_t divisors leave both members in a defined state while
fallback remains enabled. Preserve the existing handling for valid, zero, and
negative divisors.
- Around line 78-90: Reset fallback to false at the beginning of
computeScalars() before evaluating the divisor, so recomputation after changing
from an unsupported divisor can re-enable the fast path. Add a regression test
that assigns an unsupported divisor, then a supported divisor, and verifies
fast-path scalar computation is used.
- Around line 111-136: Restrict the public operator/ and operator% templates for
FastIntDiv to integral NumIntT types using the project’s existing constraint or
enablement convention. Preserve the current integer behavior and return types
for supported numerators, while preventing floating-point numerators such as
float from selecting either overload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8dd452d7-1ae4-444d-a60a-1ad44733eaa1
📒 Files selected for processing (2)
cpp/include/raft/util/fast_int_div.cuhcpp/tests/util/fast_int_div.cu
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/bench/prims/util/fast_int_div.cu (1)
100-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse typed RAFT mdarrays for these buffers.
These are simple one-dimensional owning arrays, but two use
rmm::device_uvectorandd_divisorsuses an untyped byte buffer plus a cast. Replace them with typed RAFT mdarrays to remove byte-size/cast bookkeeping and follow the project storage idiom.As per coding guidelines, “Prefer
raftmdarray types for owning data overrmm::device_uvector,rmm::device_buffer, orstd::vectorwhen an mdarray fits the use case.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/bench/prims/util/fast_int_div.cu` around lines 100 - 102, Replace the owning buffers d_numerators, d_divisors, and out_d with one-dimensional typed RAFT mdarrays using their appropriate element types and existing resource/context. Update their construction and accesses to use mdarray storage directly, removing the untyped byte-size bookkeeping and casts while preserving the current data flow.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/bench/prims/util/fast_int_div.cu`:
- Around line 57-70: Update the benchmark data generation around the numerator
and divisor distributions to add separately named int64 fallback cases using
values above the int32 range, including divisors outside the 32-bit range.
Preserve the existing int32-ranged cases, and ensure the new cases are passed to
FastIntDiv<int64_t> so they exercise the actual fallback paths.
---
Nitpick comments:
In `@cpp/bench/prims/util/fast_int_div.cu`:
- Around line 100-102: Replace the owning buffers d_numerators, d_divisors, and
out_d with one-dimensional typed RAFT mdarrays using their appropriate element
types and existing resource/context. Update their construction and accesses to
use mdarray storage directly, removing the untyped byte-size bookkeeping and
casts while preserving the current data flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 57e24140-6188-4986-94fb-db80b918f127
📒 Files selected for processing (3)
cpp/bench/prims/CMakeLists.txtcpp/bench/prims/util/fast_int_div.cucpp/include/raft/util/fast_int_div.cuh
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/include/raft/util/fast_int_div.cuh
@mfoerste4 Thank you for emphasizing this point. I added the analysis to prove that in the PR description. |
divyegala
left a comment
divyegala
left a comment
There was a problem hiding this comment.
PR looks good, just a docs request
| template <typename NumIntT, typename DivIntT> | ||
| HDI std::common_type_t<NumIntT, DivIntT> operator/(NumIntT n, const FastIntDiv<DivIntT>& divisor) |
There was a problem hiding this comment.
Could you please add usage docs on the class and the operator overloads, and mention the overloads aren't meant to be used directly?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/include/raft/util/fast_int_div.cuh (2)
69-78: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInitialize all state in
computeScalars().When
d > kInt32Max,computeScalars()sets onlyfallback. Copying the resultingFastIntDiv<int64_t>reads indeterminatemandp; the benchmark performs such copies while buildingh_divisors. Also resetfallbackwhen assigning a supported divisor, or a reused object remains on the fallback path. Resetfallback,m, andpbefore the divisor checks, and add tests for copy and reassignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/raft/util/fast_int_div.cuh` around lines 69 - 78, Update FastIntDiv::computeScalars() to reset fallback, m, and p before divisor checks, ensuring the d > kInt32Max fallback path leaves all state initialized. When computing a supported divisor, explicitly clear fallback so reused objects return to the normal path. Add tests covering copy construction and reassignment after both fallback and supported-divisor computations.Sources: Coding guidelines, Path instructions
121-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject non-integral numerators at compile time.
double n = 5.5; n / FastIntDiv<int32_t>(2)returns2, not2.75, because the fast path castsntoint64_t. The%overload accepts the same unsupported numerator type. Add an integral constraint to both overloads and document the numerator requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/raft/util/fast_int_div.cuh` around lines 121 - 139, Constrain both FastIntDiv operator/ and operator% overloads to integral numerator types so non-integral arguments fail at compile time instead of entering the integer fast path. Apply the constraint using the existing template/type-trait conventions, and update the nearby numerator documentation to state that NumIntT must be integral.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/include/raft/util/fast_int_div.cuh`:
- Around line 69-78: Update FastIntDiv::computeScalars() to reset fallback, m,
and p before divisor checks, ensuring the d > kInt32Max fallback path leaves all
state initialized. When computing a supported divisor, explicitly clear fallback
so reused objects return to the normal path. Add tests covering copy
construction and reassignment after both fallback and supported-divisor
computations.
- Around line 121-139: Constrain both FastIntDiv operator/ and operator%
overloads to integral numerator types so non-integral arguments fail at compile
time instead of entering the integer fast path. Apply the constraint using the
existing template/type-trait conventions, and update the nearby numerator
documentation to state that NumIntT must be integral.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c745c517-c528-4c3f-bc50-cdd3db722730
📒 Files selected for processing (1)
cpp/include/raft/util/fast_int_div.cuh
|
/merge |
3 participants
This PR allows FastIntDiv to support the full 64-bit range values. Within the 32-bit range values, the fast implementation is selected. When exceeds 32-bit, it fallbacks during runtime to the normal division. The overhead of runtime check is measured to be negligible.
FastIntDiv is used by in-place gather() where number of columns (divisor) is set once and the / and % divisions are called a lot more frequent to obtain row and col numbers from the given flat index. Therefore, the benchmark compares the fast int division against the normal division at 100 divisors and 1 million numerators for 32-bit and 64-bit range.
@mfoerste4 Original version without the current changes and this PR -> Only negligible runtime overhead (~4%) is added.
The gather() bench gives a similar comparison. The added runtime check is only a negligible overhead