Skip to content

[xnn update prep] deprecate sdpa #11506

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
Merged
merged 12 commits into from
Jun 26, 2025
Merged

[xnn update prep] deprecate sdpa #11506

merged 12 commits into from
Jun 26, 2025

Conversation

mcr229
Copy link
Contributor

@mcr229 mcr229 commented Jun 9, 2025 •
edited
Loading

Differential Revision: D77265464

mcr229 added 2 commits June 9, 2025 15:10
@mcr229
Update
ad82d03 
[ghstack-poisoned]
@mcr229
Update
425ca7e 
[ghstack-poisoned]
@mcr229 mcr229 requested a review from digantdesai as a code owner June 9, 2025 22:10
@mcr229
Copy link
Contributor Author

mcr229 commented Jun 9, 2025 •
edited
Loading

Stack from ghstack (oldest at bottom):

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Jun 9, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11506

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

❌ 4 New Failures, 7 Pending, 1 Unrelated Failure

As of commit 6e7028e with merge base d4cc258 (image):

NEW FAILURES - The following jobs have failed:

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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 Jun 9, 2025
This was referenced Jun 9, 2025
Merged
Merged
@mcr229 mcr229 added the release notes: none Do not include this in the release notes label Jun 9, 2025
digantdesai
digantdesai requested changes Jun 10, 2025
View reviewed changes
backends/xnnpack/runtime/XNNCompiler.cpp
@@ -2097,7 +2061,6 @@ DefineNodeFunc getDefineNodeFunc(fb_xnnpack::XNodeUnion nodeType) {
_DEFINE(Concatenate4)
_DEFINE(Concatenate5)
_DEFINE(StaticSlice)
_DEFINE(ScaledDotProductAttention)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not updating schema for marking deprecated? XNNScaledDotProductAttention

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so XNNPACK removed the operator from their codebase, so for next update we need to delete. I can mark the operator in the schema as deprecated though.

backends/xnnpack/operators/op_sdpa.py
@@ -1,111 +0,0 @@
# Copyright (c) Meta Platforms, Inc. and affiliates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we continue partitioning this and lower it as decomposed?
Else this will be a BC breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So ever since my new partitioner, SDPA has not been delegated. I assumed that no models in production or since h ave really used any sdpa implementation (also because i heard our sdpa is slow). so I believe this is safe to remove. I can import it internally to make sure

Copy link
Contributor

Choose a reason for hiding this comment

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

What I was thinking was we should handle decomp of SDPA inside XNNPACK AoT so that we don't regress for current perf. Lowering pieces is OK but can cause unexpected perf drops with slight changes.
And I guess when the constraint fails it will get decomp and we will lower it in pieces anyway, right?

Base automatically changed from gh/mcr229/28/head to main June 10, 2025 16:52
@facebook-github-bot
Copy link
Contributor

@mcr229 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mcr229
Update
77e18e6 
[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

@mcr229 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mcr229
Update
a485b3b 
[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

@mcr229 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@mcr229
Update
f3822ec 
[ghstack-poisoned]
@mcr229 mcr229 mentioned this pull request Jun 23, 2025
Merged
@mcr229
Copy link
Contributor Author

mcr229 commented Jun 24, 2025

@mcr229 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kedarnath03 pushed a commit to kedarnath03/executorch that referenced this pull request Jun 25, 2025
@mcr229
[xnn update prep] deprecate sdpa
bed6bfb 
ghstack-source-id: 479c94a
ghstack-comment-id: 2957149191
Pull Request resolved: pytorch/executorch#11506
kedarnath03 pushed a commit to kedarnath03/executorch that referenced this pull request Jun 25, 2025
@mcr229
[xnn update prep] deprecate sdpa
49cf53c 
ghstack-source-id: 479c94a
ghstack-comment-id: 2957149191
Pull Request resolved: pytorch/executorch#11506
@mcr229
Update
3ee55d5 
[ghstack-poisoned]
mcr229 added a commit to mcr229/executorch that referenced this pull request Jun 25, 2025
@mcr229
[xnn update prep] deprecate sdpa
baa1607 
ghstack-source-id: 929a8bc
ghstack-comment-id: 2957149191
Pull Request resolved: pytorch#11506
@mcr229
Copy link
Contributor Author

mcr229 commented Jun 26, 2025

@mcr229 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

digantdesai
digantdesai approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@digantdesai digantdesai left a comment

Choose a reason for hiding this comment

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

Stamping to unblock rebase, but we should revisit this under the larger task of replacing custom SDPA op with XNNPACK's in the future.

@mcr229 mcr229 merged commit d35e3a2 into main Jun 26, 2025
95 of 104 checks passed
@mcr229 mcr229 deleted the gh/mcr229/29/head branch June 26, 2025 20:30
swolchok
swolchok reviewed Jun 30, 2025
View reviewed changes
kernels/portable/cpu/util/dtype_util.cpp
@@ -27,8 +27,6 @@ bool check_tensor_dtype(
return executorch::runtime::tensor_is_floating_type(t);
case SupportedTensorDtypes::INTB:
return executorch::runtime::tensor_is_integral_type(t, true);
case SupportedTensorDtypes::BOOL:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change here? bad rebase?

swolchok added a commit that referenced this pull request Jun 30, 2025
@swolchok
Reapply changes reverted by #11506
9062090 
This change seems to have accidentally reverted several of my changes due to (presumably) a bad rebase.

Differential Revision: [D77559984](https://our.internmc.facebook.com/intern/diff/D77559984/)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jul 2, 2025
@swolchok
Update base for kick tests on "Reapply changes reverted by #11506"
c59ddf7 
This change seems to have accidentally reverted several of my changes due to (presumably) a bad rebase.

Differential Revision: [D77559984](https://our.internmc.facebook.com/intern/diff/D77559984/)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jul 2, 2025
@swolchok
kick tests on "Reapply changes reverted by #11506"
6f14cdf 
This change seems to have accidentally reverted several of my changes due to (presumably) a bad rebase.

Differential Revision: [D77559984](https://our.internmc.facebook.com/intern/diff/D77559984/)

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jul 2, 2025
@swolchok
Reapply changes reverted by #11506
86056cd 
Pull Request resolved: #12121

This change seems to have accidentally reverted several of my changes due to (presumably) a bad rebase.
ghstack-source-id: 293898720
@exported-using-ghexport

Differential Revision: [D77559984](https://our.internmc.facebook.com/intern/diff/D77559984/)
facebook-github-bot pushed a commit that referenced this pull request Jul 2, 2025
@swolchok
Reapply changes reverted by #11506
78b54ca 
Differential Revision: D77559984

Pull Request resolved: #12121
swolchok added a commit that referenced this pull request Jul 2, 2025
@pytorchbot @swolchok
Reapply changes reverted by #11506 (#12176)
9409774 
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: #12121 by
@swolchok
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/swolchok/487/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/swolchok/487/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/swolchok/487/orig
@diff-train-skip-merge

Co-authored-by: Scott Wolchok <[email protected]>
Tanish2101 pushed a commit to Tanish2101/executorch that referenced this pull request Jul 9, 2025
@pytorchbot @swolchok @Tanish2101
Reapply changes reverted by pytorch#11506 (pytorch#12176)
4969497 
This PR was created by the merge bot to help merge the original PR into
the main branch.
ghstack PR number: pytorch#12121 by
@swolchok
^ Please use this as the source of truth for the PR details, comments,
and reviews
ghstack PR base:
https://github.com/pytorch/executorch/tree/gh/swolchok/487/base
ghstack PR head:
https://github.com/pytorch/executorch/tree/gh/swolchok/487/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/main
Merge bot PR head:
https://github.com/pytorch/executorch/tree/gh/swolchok/487/orig
@diff-train-skip-merge

Co-authored-by: Scott Wolchok <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swolchok swolchok swolchok left review comments

@digantdesai digantdesai digantdesai approved these changes

@manuelcandales manuelcandales Awaiting requested review from manuelcandales manuelcandales is a code owner

@jackzhxng jackzhxng Awaiting requested review from jackzhxng

@larryliu0820 larryliu0820 Awaiting requested review from larryliu0820

@mergennachin mergennachin Awaiting requested review from mergennachin

@lucylq lucylq Awaiting requested review from lucylq

@JacobSzwejbka JacobSzwejbka Awaiting requested review from JacobSzwejbka

@cccclai cccclai Awaiting requested review from cccclai

@Gasoonjia Gasoonjia Awaiting requested review from Gasoonjia

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: none Do not include this in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mcr229 @facebook-github-bot @swolchok @digantdesai