Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix integer overflow in remainder operator #5906

Merged
merged 4 commits into from
Mar 21, 2018
Merged

Fix integer overflow in remainder operator #5906

merged 4 commits into from
Mar 21, 2018

Conversation

vedanuj
Copy link
Contributor

@vedanuj vedanuj commented Mar 20, 2018

This PR fixes issue #5875 . Fixes integer overflow due to multiplication.

Please review @zou3519

@zou3519
Copy link
Contributor

zou3519 commented Mar 20, 2018

Nice catch, @vedanuj! From the code changes, it looks like integer overflow from the multiplication is causing the issue. Is this correct?

In the issue, the @robert-wagner mentioned two failure cases. Let x be a scalar and q be a large number.

  1. x % q = x + q
  2. x % q = q
    Could you add test cases with simple examples to test/test_torch.py (and also test/test_cuda.py) for these two cases?

I listed an example of the first failure case in the issue; do you have an example of the second failure case, @robert-wagner?

@zou3519
Copy link
Contributor

zou3519 commented Mar 20, 2018

Hmm also see the failing tests (hit Details).

@vedanuj
Copy link
Contributor Author

vedanuj commented Mar 20, 2018

@zou3519 Yes the integer overflow was causing the issue. I will add the tests and check why the test cases are failing.

@robert-wagner
Copy link
Contributor

x = torch.tensor(-23500, dtype=torch.int64)
q = 392486996410368
x % q # equal to x should be q-x

@vedanuj
Copy link
Contributor Author

vedanuj commented Mar 20, 2018 •
edited
Loading

The new commits fix the remainder operator in Cuda. Add tests in test_torch.py and test_cuda.py

Tests :

python test/test_cuda.py TestCuda.test_remainder_overflow

python test/test_torch.py TestTorch.test_remainder_overflow

zou3519
zou3519 reviewed Mar 20, 2018
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

This looks great! One minor comment

aten/src/TH/generic/THTensorMath.c Outdated
@@ -1070,7 +1070,7 @@ void THTensor_(remainder)(THTensor *r_, THTensor *t, real value)
#else
// There is no NAN for integers
rp[i] = tp[i] % value;
if (rp[i] * value < 0)
if ((rp[i] < 0) != (value < 0))

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@vedanuj
Copy link
Contributor Author

vedanuj commented Mar 21, 2018

@pytorchbot retest this please

zou3519
zou3519 approved these changes Mar 21, 2018
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

This is great! Thanks @vedanuj!

@soumith soumith merged commit 08b1324 into pytorch:master Mar 21, 2018
@soumith soumith mentioned this pull request Mar 21, 2018
Closed
jekbradbury pushed a commit to jekbradbury/pytorch that referenced this pull request Mar 21, 2018
@vedanuj @jekbradbury
Fix integer overflow in remainder operator (pytorch#5906)
df66e4c 
* Fix integer overflow in remainder

* Fix remainder operator in CUDA

* Add tests for remainder integer overflow

* Add has_different_sign static function
@vedanuj vedanuj deleted the fix_int_overflow branch March 21, 2018 06:55
@mnoukhov mnoukhov mentioned this pull request Aug 19, 2019
Closed
jfix71 added a commit to jfix71/pytorch that referenced this pull request Jan 31, 2022
@jfix71 @facebook-github-bot
[acc_ops] Move slice_tensor to consider single dim at a time (pytorch…
e07c30b 
…#5906)

Summary:
Pull Request resolved: pytorch/glow#5906

Pull Request resolved: pytorch#71883

Fixes slice_tensor retracing. Include fix for retrace coverage.

Missed in D33760455 (pytorch@66939e3).

Test Plan: CI

Reviewed By: wushirong

Differential Revision: D33802222

fbshipit-source-id: 6796d65ea79107c0fcc577b583601f7c2dd35db1
facebook-github-bot pushed a commit that referenced this pull request Jan 31, 2022
@jfix71 @facebook-github-bot
[acc_ops] Move slice_tensor to consider single dim at a time (#5906)
98fd23c 
Summary:
Pull Request resolved: pytorch/glow#5906

Pull Request resolved: #71883

Fixes slice_tensor retracing. Include fix for retrace coverage.

Missed in D33760455 (66939e3).

Test Plan: CI

Reviewed By: wushirong

Differential Revision: D33802222

fbshipit-source-id: 4e0e44ae4a4eb70b99d79f0cd582182031b87e25
pytorchmergebot pushed a commit that referenced this pull request Jan 31, 2022
@jfix71 @pytorchmergebot
[acc_ops] Move slice_tensor to consider single dim at a time (#5906)
184b78c 
Summary:
Pull Request resolved: pytorch/glow#5906

Pull Request resolved: #71883

Fixes slice_tensor retracing. Include fix for retrace coverage.

Missed in D33760455 (66939e3).

Test Plan: CI

Reviewed By: wushirong

Differential Revision: D33802222

fbshipit-source-id: 4e0e44ae4a4eb70b99d79f0cd582182031b87e25
(cherry picked from commit 98fd23c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apaszke apaszke apaszke left review comments

@zou3519 zou3519 zou3519 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vedanuj @zou3519 @robert-wagner @apaszke @soumith