Skip to content

Fix gather/scatter fast division bug#3059

Merged
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
huuanhhuyn:bug_3055
Jun 29, 2026
Merged

Fix gather/scatter fast division bug#3059
rapids-bot[bot] merged 5 commits into
NVIDIA:mainfrom
huuanhhuyn:bug_3055

Conversation

@huuanhhuyn

@huuanhhuyn huuanhhuyn commented Jun 24, 2026 •
edited by tfeher
Loading

Copy link
Copy Markdown
Contributor

raft::matrix::gather/scatter crashes at int32 boundary even when it advertises int64 type -> issue 3055. This PR fixes this issue.

int64 is still not supported for performance reason with int128.

closes #3055

@huuanhhuyn huuanhhuyn requested a review from a team as a code owner June 24, 2026 08:44
@copy-pr-bot

copy-pr-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@huuanhhuyn huuanhhuyn mentioned this pull request Jun 24, 2026
Closed
@tfeher tfeher added bug Something isn't working breaking Breaking change labels Jun 24, 2026
tarang-jain
tarang-jain approved these changes Jun 24, 2026
View reviewed changes

@tarang-jain tarang-jain left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix.

@tarang-jain

tarang-jain commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Actually, can we add a test for this diff? We could either add a test for FastIntDiv directly, or we can test the raft::matrix::gather when N * D exceeds INT_MAX. I am more inclined to have tests for both -- testing FastIntDiv should be quick where we just compare with the C++ operators ( / and %). But the more important one is to test the raft::matrix::gather for those larger matrices.

@huuanhhuyn huuanhhuyn requested a review from a team as a code owner June 26, 2026 08:23
huuanhhuyn
huuanhhuyn commented Jun 26, 2026
View reviewed changes
Comment thread cpp/tests/matrix/gather.cu Outdated
inplace_inputs_i64);
GATHER_TEST((GatherTest<false, false, true, float, int64_t, int64_t>),
GatherInplaceTestCI64I64,
inplace_inputs_i64_i32_max); // slow test, 8GB allocation, reproduces https://github.com/rapidsai/raft/issues/3055

@huuanhhuyn huuanhhuyn Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tfeher @tarang-jain Only float type is possible here (8GB allocation, 100ms). I would suggest to not have it. I already added a regression test for the fix in fast_int_div.cu

@tfeher tfeher Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is fine to spend 100ms on this.

tfeher
tfeher approved these changes Jun 26, 2026
View reviewed changes

@tfeher tfeher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Huy for the fix, LGTM!

Comment thread cpp/tests/matrix/gather.cu Outdated
inplace_inputs_i64);
GATHER_TEST((GatherTest<false, false, true, float, int64_t, int64_t>),
GatherInplaceTestCI64I64,
inplace_inputs_i64_i32_max); // slow test, 8GB allocation, reproduces https://github.com/rapidsai/raft/issues/3055

@tfeher tfeher Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is fine to spend 100ms on this.

@tfeher

tfeher commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

/ok to test 9acb86f

@aamijar aamijar requested review from a team as code owners June 26, 2026 21:37
@aamijar

aamijar commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

/ok to test 33ec5d7

@aamijar

aamijar commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

/ok to test b18e428

@aamijar

aamijar commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

/ok to test dc2ede3

@huuanhhuyn
Fix gather/scatter fast div bug
618a22a 
- gather/scatter crashes at int32 boundary even when it advertises int64 type. This change fixes this issue.
- int64 is still not supported for performance reason with int128.
@huuanhhuyn
Add unit tests for FastIntDiv
c6f82a2
@huuanhhuyn
Remote heavy UT due to OOM
180ccef
@huuanhhuyn
precommit run
9f3d2cb
@huuanhhuyn

huuanhhuyn commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 9f3d2cb

@huuanhhuyn

huuanhhuyn commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test d87dcdd

@huuanhhuyn

huuanhhuyn commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

/merge

1 similar comment
@tfeher

tfeher commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

/merge

tfeher
tfeher approved these changes Jun 29, 2026
View reviewed changes

@tfeher tfeher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tfeher

tfeher commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

/merge

1 similar comment
@gforsyth

gforsyth commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

/merge

@rapids-bot rapids-bot Bot merged commit 30cb3e0 into NVIDIA:main Jun 29, 2026
79 checks passed
@aamijar

aamijar commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Hello @huuanhhuyn, going forward let's remember to build cuvs and build cuml at the least to ensure that raft doesn't cause any breakages. This PR is causing some breakages currently.

This was referenced Jun 29, 2026
Merged
Merged
rapids-bot Bot pushed a commit to rapidsai/cuml that referenced this pull request Jun 29, 2026
@aamijar
Fix FastIntDiv tparam (#8299)
637161b 
Add explicit template parameters for FastIntDiv, follow up to NVIDIA/raft#3059.

Authors:
  - Anupam (https://github.com/aamijar)

Approvers:
  - Jim Crist-Harif (https://github.com/jcrist)

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

Reviewers

@tfeher tfeher tfeher approved these changes
@tarang-jain tarang-jain tarang-jain approved these changes

Assignees

No one assigned

Labels

breaking Breaking change bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] raft::matrix::gather (in-place overload): illegal memory access when N*D ≥ 2^31 (32-bit index overflow)

5 participants

@huuanhhuyn @tarang-jain @tfeher @aamijar @gforsyth