-
Notifications
You must be signed in to change notification settings - Fork 687
Arm backend: Update VelaIO handling #13282
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13282
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New Failure, 1 Cancelled Job, 4 Unrelated FailuresAs of commit 8581ecc with merge base 176800e ( NEW FAILURE - The following job has failed:
CANCELLED JOB - The following job was cancelled. Please retry:
BROKEN TRUNK - The following jobs 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. |
New Vela version that we hope fix #1302 |
@digantdesai new Vela version that we hope fix #13022 |
Yeah @apullin is trying out the new vela version 🙏 |
As this is not merged yet we already have an even newver vela fixing another bug, depending if we see if thei has beem brought into meta or not for testing we might just update the Vela SHA in this PR. If you want us to keep this untouch @digantdesai just let us know but in that case you will get 2 Vela update PRs to juggle merge at the same time :) |
01b3714
to
df556e6
Compare
We haven't had a chance to update Vela internally. Perhaps this week - CC @apullin If I were to guess we can just move the Vela to the newer of two SHAs, and it should be OK. |
Thanks, Yes, preferable latest Vela but note that Vela needs to be steped together with this PR, the 6D support is needed to be done on Executorch side to not break things. |
7f2a662
to
a0e1033
Compare
Just to update here - I did try out this commit and the corresponding commit it ethos-u-vela, and it did resolve the crash in the compiler for both the minimum working example and for our network that uses that pattern internally. There are some other ongoing issues with lowering, where all the layers get rejected, but that may be due to a totally separate issue than the observed crash that kicked off this fix. |
VelaIO is always 6D. - Update AOT handling of metadata from Vela. - Adds unittest to trigger 5D cases. - Updates EthosUBackend to read IO as 6D arrays. Signed-off-by: Oscar Andersson <oscar.andersson@arm.com> Change-Id: I8d7d3a44ac84e5bb14fa27e7b7765c3b7a8ee483
@oscarandersson8218 this probably need a rebase again :( now after the vela install is reworked. Sorry about that. |
e404ce1
to
8581ecc
Compare
CI failures are unrelated. @digantdesai @apullin can we merge this? It's blocking quite a few of our patches. |
@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D81806570. |
I don't think anything on our end precludes a merge ... ? I evaluated with the specific commit from this PR, and will continue to use it with that pinned version. So merging should not affect us. When this gets rolled into a release tag (non-rc), we'll probably just move on to that version. |
This is a combined version of #13282 and #14062. The changes here should mirror #14135 Differential Revision: [D81810642](https://our.internmc.facebook.com/intern/diff/D81810642/) [ghstack-poisoned]
This is a combined version of #13282 and #14062. The changes here should mirror #14135 Differential Revision: [D81810642](https://our.internmc.facebook.com/intern/diff/D81810642/) ghstack-source-id: 309399055 Pull Request resolved: #14280
Merged in #14293 |
Thanks @zingo |
VelaIO is always 6D.
Fixes #13022
cc @digantdesai @freddan80 @per @zingo