-
Notifications
You must be signed in to change notification settings - Fork 690
Arm backend: Add recipe infrastructure #14849
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
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14849
Note: Links to docs will display an error until the docs builds have been completed. ❌ 5 New Failures, 1 Unrelated FailureAs of commit 224d8d4 with merge base 8fbc42c ( 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. |
quantizer.set_global(get_symmetric_quantization_config(is_per_channel=False)) | ||
|
||
|
||
class ArmRecipeProvider(BackendRecipeProvider): |
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.
Looks great, thanks for adding these recipes, is it possible to add a couple of model tests using any of these recipes, just to make sure?
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.
Thanks, I plan on adding tests using recipes in an upcoming patch.
recipe_type, | ||
TargetRecipe(common.get_tosa_compile_spec("TOSA-1.0+FP", **kwargs)), | ||
) | ||
case ArmRecipeType.TOSA_INT8_STATIC_PER_TENSOR: |
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.
Do we need any kwarg validation, to let user know if they've configured the recipes wrong?
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.
Good idea, will do that in an upcoming patch.
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.
Have minor comments,
Recipes were introduced with issue pytorch#12248 to simplify the ET api for new users. A backend introduces standard recipes with partitioner and quantizer configuration, and possibly passes. In addition to the benefits of integrating with existing infra, the arm backend can benefit from the recipe structure as a test-framework-agnostic way of defining how a tested model should be lowered. Additonally, it ties into simplifying the aot_arm_compiler interface. Signed-off-by: Erik Lundell <[email protected]> Change-Id: Iebd3f101f94a3df657af3b49ea563f72cbca3a9a
Dry coded with AI, needs verification Signed-off-by: Erik Lundell <[email protected]> Change-Id: I79d3009c5bc6557601cab9501f2e275f89c95330
a8b1cfc
to
224d8d4
Compare
[global_int8_per_channel], | ||
) | ||
|
||
case ArmRecipeType.VGF_FP: |
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.
Nit
case ArmRecipeType.VGF_FP: | |
case ArmRecipeType.VGF_FP32: |
""" | ||
ETHOSU_U55_INT8_STATIC_PER_CHANNEL = "arm_ethosu_u55_int_static_per_channel" | ||
""" Kwargs: | ||
- macs: int = 128, |
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.
do we need these comments? I am afraid they will get out of sync without being tested.. we should have this in validation as you are planning..
edge_transform_passes: List[PassType] = field(default_factory=lambda: []) | ||
|
||
|
||
class ArmExportRecipe(ExportRecipe): |
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.
add at least a basic test to make this PR self containing..
@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D84517727. |
Recipes were introduced with issue #12248 to simplify the ET api for new users. A backend introduces standard recipes with partitioner and quantizer configuration, and possibly passes.
cc @freddan80 @per @zingo @oscarandersson8218 @digantdesai