-
Notifications
You must be signed in to change notification settings - Fork 694
Cortex-M backend: Add mul and linear tests #14746
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
Minor included fixes: - Make quantized_linear_fusion_pass an XNNPACK pass to initialize it with an exported program - Add TO_EXECUTORCH as a valid stage after RUN_PASSES - Add ramp_tensor function to simplify creating dummy data Signed-off-by: Adrian Lundell <adrian.lundell@arm.com> Change-Id: Id13be6427390483aa1df1b76fc363ae4d0eae876
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14746
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New FailuresAs of commit 048b0c0 with merge base 75f968d ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@digantdesai @psiddh What do you think? |
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, the PR as it covers all basic scenarios given the current state of Passes
xfails properly document known pass limitations
Looks like "int32 NameError" specifically happens in the dialect transformation stage for these two test cases: linear_rank2_pos and linear_bias. This appears more fundamental issue , isn't ? |
I think it has to do with how meta-data is propagated in the pass if I remember correctly, the error is fixed in an upcoming patch at least! |
Signed-off-by: Adrian Lundell <adrian.lundell@arm.com>
def __init__(self): | ||
super().__init__( | ||
XNNPACKPassManager, pass_list=[QuantizedOpFusionPass, ReplaceQuantNodesPass] | ||
XNNPACKPassManager, |
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.
Let's fork it and put it under backends/cortex_m
I was thinking s/XNNPACK/MCU
but we can do the rebranding later and if you want to stick to cortex_m then s/XNNPACK/CortexM
?
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 went with CortexM in here 57a7903 so it will have to be rebranded later
@@ -0,0 +1,211 @@ | |||
# Copyright 2025 Arm Limited and/or its affiliates. |
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.
❤️
self.check_count(ops_before_transforms) | ||
self.run_passes() | ||
self.check_count(ops_after_transforms) | ||
self.run_method_and_compare_outputs(inputs=self.example_inputs, qtol=qtol) |
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.
should we merge test_dialect
and test_impl
?
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 kind of like the split since the python implementation is so separate from the c++ implementation and it's nice to get feedback on only the thing you are working on. On the other hand you can simply modify the stages of the tester during development to get this so it should be fine!
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.
Given, (1) python eager dq->op->q, (2) python impl of the cortex-m op, and (3) C++ impl of the cortex-m op, I guess it makes sense to do A: (1) vs. (2) and B: (1) vs. (3) but when I suggested to merge I assumed we don't really care about (2) besides unit-tests, unless the second comparison (i.e. B) is (2) vs. (3).
Minor included fixes:
cc @freddan80 @per @zingo @oscarandersson8218 @digantdesai @psiddh