Skip to content

FastIntDiv: fallback to normal division when values exceed 32-bit ran…#3093

Open
huuanhhuyn wants to merge 1 commit into
NVIDIA:mainfrom
huuanhhuyn:fastintdiv_64
Open

FastIntDiv: fallback to normal division when values exceed 32-bit ran…#3093
huuanhhuyn wants to merge 1 commit into
NVIDIA:mainfrom
huuanhhuyn:fastintdiv_64

Conversation

@huuanhhuyn

@huuanhhuyn huuanhhuyn commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

FastIntDiv: Fallback to the normal division when division values exceed 32-bit range.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved integer division and remainder handling for values outside the 32-bit range.
    • Added safer fallback behavior for large 64-bit operands.
    • Maintained correct behavior for supported positive divisors and invalid divisor cases.
  • Tests

    • Expanded coverage for 32-bit and 64-bit division and remainder results.
    • Added boundary-value and native-operation comparisons to improve reliability.

Walkthrough

Changes

The FastIntDiv division and modulo operators now fall back to native arithmetic for values using upper 32 bits. Tests explicitly cover invalid denominators and native-result parity for int32_t and int64_t.

FastIntDiv behavior and validation

Layer / File(s) Summary
Native division and modulo fallbacks
cpp/include/raft/util/fast_int_div.cuh
Updates signed-divisor documentation and adds native / and % fallbacks when operands contain upper 32-bit content.
Arithmetic and construction tests
cpp/tests/util/fast_int_div.cu
Replaces typed tests with explicit 32-bit and 64-bit cases, including invalid denominator construction and comparisons with native arithmetic.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: vyasr

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the main change: FastIntDiv falls back to normal division when values exceed 32-bit.
Description check ✅ Passed The description matches the PR objective by describing runtime fallback when values exceed the 32-bit range.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between aa9a6d5 and aeeeab0.

📒 Files selected for processing (2)
  • cpp/include/raft/util/fast_int_div.cuh
  • cpp/tests/util/fast_int_div.cu

Comment on lines +102 to 105
if ((int64_t(divisor.d) >> 32) != 0 || (int64_t(n) >> 32) != 0) return n / divisor.d;
if (divisor.d == 1) return n;
IntT ret = (int64_t(divisor.m) * int64_t(n)) >> divisor.p;
if (n < 0) ++ret;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Check the signed 32-bit boundary, not only bits 63:32.

The current check allows positive int64_t values above INT32_MAX into the fast path. For example, n = UINT32_MAX and d = 2 can overflow the signed int64_t(m) * int64_t(n) multiplication, while a divisor just above INT32_MAX can produce an incorrect quotient and remainder. Gate both operators on the actual supported range (or fall back for any value above INT32_MAX), then add regression vectors for these boundaries.

As per path instructions, the fallback must cover all overflow-risk paths and match native division/modulo across the relevant ranges.

Suggested fix
+#include <limits>
+
-  if ((int64_t(divisor.d) >> 32) != 0 || (int64_t(n) >> 32) != 0) return n / divisor.d;
+  if (n < 0 || n > std::numeric_limits<int32_t>::max() ||
+      divisor.d > std::numeric_limits<int32_t>::max()) {
+    return n / divisor.d;
+  }

Apply the equivalent condition to operator%.

Also applies to: 119-123

🤖 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 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.

Source: Path instructions

Comment on lines +25 to +39
TEST(FastIntDivTest, CompareWithNativeDivisionInt32)
{
std::vector<int32_t> magnitudes{0, 1, 2, 3, 7, 13, 255, 12345, (1 << 20), kInt32Max};
std::vector<int32_t> divisors{1, 2, 4, 7, 16, 31, 63, 128, 1000, (1 << 15), kInt32Max};

for (int32_t d : divisors) {
FastIntDiv fid(d);
for (int32_t mag : magnitudes) {
for (int32_t n : {mag, -mag}) {
ASSERT_EQ(n / fid, n / d) << "operator/ mismatch for numerator=" << n << " divisor=" << d;
ASSERT_EQ(n % fid, n % d) << "operator% mismatch for numerator=" << n << " divisor=" << d;
}
}
}
};

using FastIntDivTypes = ::testing::Types<int32_t, int64_t>;
TYPED_TEST_CASE(FastIntDivTest, FastIntDivTypes);

TYPED_TEST(FastIntDivTest, CompareWithNativeDivision) { this->CompareWithNativeDivision(); }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Exercise the signed minimum numerators explicitly.

The {mag, -mag} construction never tests INT32_MIN or INT64_MIN. Add those values directly; they are important native-fallback boundaries and cannot be produced by negating the positive maximum without overflow.

As per path instructions, updated tests must cover boundary/extreme-value vectors.

Also applies to: 41-54

🤖 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/tests/util/fast_int_div.cu` around lines 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.

Source: Path instructions

Comment on lines +41 to +54
TEST(FastIntDivTest, CompareWithNativeDivisionInt64)
{
for (int64_t d : {129, 772, 1000}) {
std::vector<int64_t> magnitudes{
0, 1013, 10007, int64_t(kInt32Max) + 3, (int64_t(1) << 33), (int64_t(3) << 40), kInt64Max};
std::vector<int64_t> divisors{
1, 129, 772, 1000, (int64_t(1) << 31), int64_t(kInt32Max) + 1, int64_t(3) << 40, kInt64Max};

for (int64_t d : divisors) {
FastIntDiv fid(d);
for (int64_t n : {1LL << 31, 2147704000LL, 3LL << 30}) { // in [2^31, 2^32)
ASSERT_EQ(n / fid, n / d) << "operator/ mismatch for numerator=" << n << " divisor=" << d;
ASSERT_EQ(n % fid, n % d) << "operator% mismatch for numerator=" << n << " divisor=" << d;
for (int64_t mag : magnitudes) {
for (int64_t n : {mag, -mag}) {
ASSERT_EQ(n / fid, n / d) << "operator/ mismatch for numerator=" << n << " divisor=" << d;
ASSERT_EQ(n % fid, n % d) << "operator% mismatch for numerator=" << n << " divisor=" << d;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add a regression vector just beyond the signed 32-bit boundary.

The 64-bit test includes INT32_MAX + 1, but not values such as INT32_MAX + 2 or UINT32_MAX that expose the current >> 32 fallback bug. Add those cases and compare both / and % with native operations.

As per path instructions, tests must include boundary/extreme-value vectors and compare against native behavior.

🤖 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/tests/util/fast_int_div.cu` around lines 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.

Source: Path instructions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant