Skip to content

Insert transposes around view_copy ops #6435

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 2 commits into from
Oct 25, 2024
Merged

Insert transposes around view_copy ops #6435

merged 2 commits into from
Oct 25, 2024

Conversation

Erik-Lundell
Copy link
Collaborator

Change-Id: Ic15088557542ff6c3c97bda2cb0bd3ab642e00ef

Change-Id: I0c64f6f92d4173420af0a6cf2d707c63beeeaa00

@oscarandersson8218 @Erik-Lundell
Insert transposes around view_copy ops
4bfe8b9 
Signed-off-by: Oscar Andersson <[email protected]>
Change-Id: Ic15088557542ff6c3c97bda2cb0bd3ab642e00ef

Signed-off-by: Erik Lundell <[email protected]>
Change-Id: I0c64f6f92d4173420af0a6cf2d707c63beeeaa00
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Oct 22, 2024 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 6294ab5 with merge base 0aa802d (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 Oct 22, 2024
@Erik-Lundell Erik-Lundell added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm ciflow/trunk and removed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 22, 2024
@digantdesai
Copy link
Contributor

curious if there is a copy cost of doing these? is there a value in optimizing these nodes we are inserting here

digantdesai
digantdesai approved these changes Oct 22, 2024
View reviewed changes
backends/arm/test/ops/test_layer_norm.py Outdated
@@ -148,7 +148,8 @@ def test_layer_norm_tosa_BI(
self.LayerNorm(*model_params), (test_data,)
)

@parameterized.expand(test_data_suite)
# Skip last test since it requires transpose.
Copy link
Contributor

Choose a reason for hiding this comment

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

and U55 doesn't support it.

@facebook-github-bot
Copy link
Contributor

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

@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 Oct 22, 2024
@Erik-Lundell
Update test_layer_norm.py
6294ab5 
Skip an additional test
@Erik-Lundell
Copy link
Collaborator Author

curious if there is a copy cost of doing these? is there a value in optimizing these nodes we are inserting here

There is a copy cost, and we also want to minimize transpose use since it's not always supported. I came up with a criteria that is a bit smarter than always inserting them in this PR but it might be possible to reduce them even more. We have discussed introducing some way of lazingly transposing just when it's needed but we are focusing on getting things functional first.

@digantdesai
Copy link
Contributor

curious if there is a copy cost of doing these? is there a value in optimizing these nodes we are inserting here

There is a copy cost, and we also want to minimize transpose use since it's not always supported. I came up with a criteria that is a bit smarter than always inserting them in this PR but it might be possible to reduce them even more. We have discussed introducing some way of lazingly transposing just when it's needed but we are focusing on getting things functional first.

Sounds good. We do something greedy on XNNPACK side to reduce number of format switches.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot merged commit cbfdf78 into pytorch:main Oct 25, 2024
107 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digantdesai digantdesai digantdesai approved these changes

Assignees
No one assigned
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Erik-Lundell @digantdesai @facebook-github-bot @oscarandersson8218