-
Notifications
You must be signed in to change notification settings - Fork 564
Support rsqrt in XNNPACK backend #7992
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
Conversation
Stack from ghstack (oldest at bottom): |
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7992
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 3e124c0 with merge base 6fcce78 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@pytorchbot label "topic: not user facing" |
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.
LGTM. Thanks @swolchok for this quick fix! @mcr229 and @digantdesai could you take a look?
@swolchok you mentioned rsqrt was used since Llama2. I'm wondering how it would help the overall performance. Probably it's fine because it's not a performance bottleneck there. It's used in RMSNorm, where the sqrt is applied to a scalar of RMS(x), not on each element of x. |
|
||
def _test_rsqrt(self, inputs): | ||
( | ||
Tester(self.Rsqrt(), inputs) |
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.
Nit: Do you want to improve this test by adding dynamic_shape support like this?
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.
improve this test by adding dynamic_shape support
it's a pointwise operator, why does it care about the shape? Everything is the same as sqrt including the test
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.
Yeah, just checking boxes, i.e. adding dynamic shape test for every operator.
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.
out of scope for this change! :)
I think I updated everywhere that needs updating? Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py
I think I updated everywhere that needs updating? Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py
I think I updated everywhere that needs updating?
Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py