-
Notifications
You must be signed in to change notification settings - Fork 683
Cortex_m backend: Add cortex_m tester + test_add #14510
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
Note that tests are currently failing, comparing for example the call to arm_elementwise_add_s8 in op_quantized_add.cpp https://github.com/pytorch/executorch/blob/ab3100715afd21e5f0cee48675d9187152775d86/backends/cortex_m/ops/op_quantized_add.cpp#L88 with the definition in CMSIS-NN https://github.com/ARM-software/CMSIS-NN/blob/88f1982a69c00ed13dd633a63da1009c48abbb4d/Include/arm_nnfunctions.h#L1923 it seems that the args are listed in the wrong order. This will be fixed in a future patch. Minor fixes to get this to work: - Add init file to make test names unique - Update conftest to not crash is_option_enabled for tests running from external folder Signed-off-by: Adrian Lundell <adrian.lundell@arm.com> Change-Id: I7962fea42994d51f871c8789b0d58b98d60a2739
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14510
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New FailuresAs of commit 364ef50 with merge base ab31007 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
from executorch.backends.test.suite.operators.test_add import Model, ModelAlpha | ||
|
||
|
||
class SelfAdd(torch.nn.Module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class SelfAdd(torch.nn.Module): | |
class SelfCortexMAdd(torch.nn.Module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added CortexM as a prefix to all models here instead, I prefer to keep to one same naming scheme and not put the target description part of the name (CortexM) in the middle of the model description part (SelfAdd).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's get the ball rolling.. thanks.
Signed-off-by: Adrian Lundell <adrian.lundell@arm.com> Change-Id: Ib98d19bf53f26c1dec103b9e875080b495c6fbaf
I have not added buck targets in this PR since it is a new structure which makes it difficult to guess how it should look, but if you add it to this one I can follow that pattern for upcoming PR:s. |
Signed-off-by: Adrian Lundell <adrian.lundell@arm.com>
@psiddh - do you want to introduce buck targets here? |
@AdrianLundell Once you land this PR , I can add the buck targets to it as a follow up |
Note that tests are currently failing, comparing for example the call to arm_elementwise_add_s8 in op_quantized_add.cpp
executorch/backends/cortex_m/ops/op_quantized_add.cpp
Line 88 in ab31007
https://github.com/ARM-software/CMSIS-NN/blob/88f1982a69c00ed13dd633a63da1009c48abbb4d/Include/arm_nnfunctions.h#L1923 it seems that the args are listed in the wrong order. This will be fixed in a future patch.
Minor fixes to get this to work:
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218