FastIntDiv: fallback to normal division when values exceed 32-bit ran… - #3093
FastIntDiv: fallback to normal division when values exceed 32-bit ran…#3093huuanhhuyn wants to merge 4 commits into
Conversation
…ge during run time.
📝 WalkthroughWalkthroughChanges
FastIntDiv behavior and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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
| * an `IntT` numerator. | ||
| */ | ||
| template <typename IntT, typename DivisorT> | ||
| RAFT_KERNEL divmod_kernel(const IntT* numerators, |
There was a problem hiding this comment.
TODO: bench in-place gather() and scatter() runtime before and after the overflow bug fix
1 participant
FastIntDiv: Fallback during runtime to the normal division when division values exceed 32-bit range.