-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Adding gpuAtomicMin and gpuAtomicMax for floating types #74225
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.
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
Adding gpuAtomicMin and gpuAtomicMax for floating types #74225
Conversation
[ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
|
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 8b6e709 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
| a[i] = 2; | ||
| sum[i] = 2; | ||
| answer[i] = pow(sum[i], static_cast<T>(factor)); | ||
| answer[i] = pow(sum[i], static_cast<T>(factor + 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fixing a preexisting bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since there are factor threads where gpuAtomicMul(a[i], sum[i]) is done the final result should be 2^5 = 2^factor+1.
Are these tests run in CI? This was failing when I ran the test locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we open a separate issue and remove this change from this PR. This is some indication that these the tests aren't running / their signal is ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, this failure isn't detected in CI because cuda_atomic_ops_test is not in run_tests.sh, fixing in #74482
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
|
@pytorchbot merge this please |
|
@pytorchbot merge this |
|
Hey @mikaylagawarecki. |
Summary: Pull Request resolved: #74225 Approved by: https://github.com/ngimel Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/4025ca8cf457889672a68e13595b1ecc59a82b34 Reviewed By: malfet Differential Revision: D35118958 Pulled By: mikaylagawarecki fbshipit-source-id: 0d12e88149c3e378fa6c772245ee3e77b23274f2
Pull Request resolved: #74225 Approved by: https://github.com/ngimel
Stack from ghstack: