Skip to content

Conversation

@AdrianLundell
Copy link
Collaborator

@AdrianLundell AdrianLundell commented Jul 30, 2025

  • Moves the needed input/output transposes into the delegated graph to run on Ethos-U rather than requiring the EthosUBackend to implement transposes on CPU.

  • Renames the annotate_channels_last_dim_order_pass to to_tosa_memory_format_pass since to be more descriptive and future proof.

This changes additionally enables running multiple batches since the EthosU transpose supports that natively, whereas the CPU implementation did not.

cc @digantdesai @freddan80 @per @zingo @oscarandersson8218

@AdrianLundell AdrianLundell added partner: arm For backend delegation, kernels, demo, etc. from the 3rd-party partner, Arm ciflow/trunk release notes: arm Changes to the ARM backend delegate labels Jul 30, 2025
@pytorch-bot
Copy link

pytorch-bot bot commented Jul 30, 2025

🔗 Helpful Links

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

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

❌ 5 New Failures, 1 Unrelated Failure

As of commit 8c66a7b with merge base d87766e (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

@meta-cla meta-cla 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 Jul 30, 2025
@AdrianLundell
Copy link
Collaborator Author

@digantdesai This is a quite big change, do you need to run it internally before merging?

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.

Glad to see so many xfails removed. Left some comments.

@common.parametrize("module", modules)
def test_to_tosa_memory_format_tosa_MI_functional(module):
# Also run the actual pass pipeline to ensure functional correctness.
pipeline = TosaPipelineMI[input_t](module, module.get_inputs(), [])
Copy link
Contributor

Choose a reason for hiding this comment

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

No FVP tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My intention here was to only test the logic of the pass with as little other things that could go wrong as possible ( quantization and fvps), we do also test the pass on FVP in other places since the change affects all modules we run tests on. Sounds ok?

@zingo
Copy link
Collaborator

zingo commented Jul 31, 2025

This will unfortunately need a rebase after the TOSA 0.80.1 removal :(

- Moves the needed input/output transposes into the
  delegated graph to run on Ethos-U rather than requiring
  the EthosUBackend to implement transposes on CPU.

- Renames the annotate_channels_last_dim_order_pass to
  to_tosa_memory_format_pass since to be more descriptive
  and future proof.

This changes additionally enables running muliple batches
since the EtohsU transpose supports that natively, whereas
the CPU implementation did not.

Change-Id: I676e5915b15cbcc370a03d70bfa2ea2fe20b2210
Signed-off-by: Adrian Lundell <[email protected]>
@facebook-github-bot
Copy link
Contributor

@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D79720805.

@Sebastian-Larsson
Copy link
Collaborator

@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D79720805.

@digantdesai Any update on this?

@digantdesai
Copy link
Contributor

Hey @Sebastian-Larsson Apologies for the delay. Internal tests got broken couple of days back so they are masking this signal once I merge #13221 hopefully I we can rebase this and pull internally again. 🙏

@facebook-github-bot
Copy link
Contributor

@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D79720805.

AdrianLundell and others added 2 commits August 21, 2025 13:05
Change-Id: Ib9e678c9471b861df141becfd2eb9511964fb627
Signed-off-by: Adrian Lundell <[email protected]>
@digantdesai
Copy link
Contributor

digantdesai commented Aug 21, 2025

Exception: An error occurred when running the 'ToTosaMemoryFormatPass'

Getting this on the internal CI
Haven't looked deeper yet

@AdrianLundell
Copy link
Collaborator Author

Exception: An error occurred when running the 'ToTosaMemoryFormatPass'

Getting this on the internal CI Haven't looked deeper yet

Have you tried the latest version? I have fixed one issue from the moving of the transpose to TOSA dialect change and that passed our internal CI at least.

@AdrianLundell
Copy link
Collaborator Author

The sigmoid error can be seen on other PR:s as well so it is not related to this patch.

@facebook-github-bot
Copy link
Contributor

@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D79720805.

@AdrianLundell
Copy link
Collaborator Author

I see that you are getting internal errors, anything I can help with?

@digantdesai
Copy link
Contributor

Actually its ok. Feel free to merge.

@oscarandersson8218 oscarandersson8218 merged commit 51de606 into pytorch:main Aug 26, 2025
588 of 602 checks passed
@digantdesai
Copy link
Contributor

Seems like it broke backends/arm/test/ops/test_sigmoid_16bit.py::test_sigmoid_tosa_INT_add_sigmoid[rand]?

@AdrianLundell
Copy link
Collaborator Author

Seems like it yes, I thought it was flakyness since we had similar accuracy errors in the sigmoids before but apparently it always fails, sorry about that.

It was previously an xfail though so it is just a matter of adding it back, I can do it first thing tomorrow morning if you do not do it.

mansnils pushed a commit that referenced this pull request Aug 27, 2025
Fix issue introduced in #12994

Signed-off-by: Adrian Lundell <[email protected]>
@AdrianLundell AdrianLundell deleted the change-1031069 branch September 4, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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 release notes: arm Changes to the ARM backend delegate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants