Skip to content

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

Merged
merged 2 commits into from
Jan 28, 2025
Merged

Support rsqrt in XNNPACK backend #7992

merged 2 commits into from
Jan 28, 2025

Conversation

swolchok
Copy link
Contributor

I think I updated everywhere that needs updating?

Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py

[ghstack-poisoned]
@swolchok
Copy link
Contributor Author

swolchok commented Jan 28, 2025

Stack from ghstack (oldest at bottom):

Copy link

pytorch-bot bot commented Jan 28, 2025

🔗 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 Failures

As of commit 3e124c0 with merge base 6fcce78 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 28, 2025
swolchok added a commit that referenced this pull request Jan 28, 2025
I think I updated everywhere that needs updating?

Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py

ghstack-source-id: 196e152
ghstack-comment-id: 2617426445
Pull Request resolved: #7992
@iseeyuan
Copy link
Contributor

@pytorchbot label "topic: not user facing"

Copy link
Contributor

@iseeyuan iseeyuan left a 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?

@iseeyuan
Copy link
Contributor

iseeyuan commented Jan 28, 2025

@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)
Copy link
Contributor

@digantdesai digantdesai Jan 28, 2025

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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! :)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 28, 2025
I think I updated everywhere that needs updating?

Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py

ghstack-source-id: edaa6b4
ghstack-comment-id: 2617426445
Pull Request resolved: #7992
@swolchok swolchok merged commit d30a0ca into main Jan 28, 2025
45 checks passed
@swolchok swolchok deleted the gh/swolchok/212/head branch January 28, 2025 17:33
YIWENX14 pushed a commit that referenced this pull request Jan 28, 2025
I think I updated everywhere that needs updating?

Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py
zonglinpeng pushed a commit to zonglinpeng/executorch that referenced this pull request Jan 30, 2025
I think I updated everywhere that needs updating?

Test Plan: python -m unittest backends/xnnpack/test/ops/test_rsqrt.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants