-
Notifications
You must be signed in to change notification settings - Fork 695
Arm backend: Convert ExportPasses to ArmPasses #14721
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
Arm backend: Convert ExportPasses to ArmPasses #14721
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14721
Note: Links to docs will display an error until the docs builds have been completed. ❌ 7 New Failures, 3 Unrelated FailuresAs of commit ca3e577 with merge base 57a7903 ( NEW FAILURES - The following jobs have failed:
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. |
|
@pytorchbot label ciflow/trunk |
|
@pytorchbot label "partner: arm" |
This PR needs a
|
4bb2df4 to
ab01d4a
Compare
| class ArmPass(ExportPass): | ||
| """Base class for Arm passes""" | ||
|
|
||
| def __init__(self, exported_program: Optional[torch.export.ExportedProgram] = None): |
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 think this is a step in the right direction.
Just a word of caution for the passes which do use exported_program mainly with an intention to add modes which ends up updating the graph signature i.e. using utils like, create_constant_placeholder is that if the graph inside the exported program and the graph the pass got are out of sync then all sorts of weird errors can happen. So it might be wise to assert something like id(exported_progra.graph_module) == if(pass_arg_graph_module)
All passes in backends/arm/_passes have been set to inherit from `ArmPass` instead of `ExportPass`. Furthermore, the attribute `exported_program` in `ArmPass` has been removed to instead let each subclasses set it. The reason is that it was type annotated as an `Optional[ExportedProgram]]` in `ArmPass` and Mypy complained about that when a subclass tried to access it without checking for `None`. By moving the attribute down to each subclass that needs it, the confusion around whether the value is `None` or not is elimininated. Signed-off-by: Martin Lindström <[email protected]> Change-Id: Ic73ecbeca255dfca74e23b4ce422dc06a094a058
ab01d4a to
ca3e577
Compare
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.
Unrelated CI failures
All passes in backends/arm/_passes have been set to inherit from
ArmPassinstead ofExportPass.Furthermore, the attribute
exported_programinArmPasshas been removed to instead let each subclasses set it. The reason is that it was type annotated as anOptional[ExportedProgram]]inArmPassand Mypy complained about that when a subclass tried to access it without checking forNone. By moving the attribute down to each subclass that needs it, the confusion around whether the value isNoneor not is elimininated.Test plan
The persistent tests in the Arm backend should cover this change.
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218