-
Notifications
You must be signed in to change notification settings - Fork 684
Arm backend: Move rescale ops out of node visitors #14584
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/14584
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 1 Unrelated FailureAs of commit 0b65f38 with merge base f81e834 ( 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. |
@pytorchbot label ciflow/trunk |
@pytorchbot label "partner: arm" |
failures look unrelated |
dtype=torch.int32, | ||
) | ||
|
||
if target in [ |
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.
may be this boat has sailed already but for logical grouping perspective, I would rather keep the util call in respective ops instead of a pass.. but that's just me. Not a strong objection.
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.
@digantdesai Not sure what you mean fully. Can you elaborate a bit?
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.
Instead of inserting rescale ops in the tosa graph through this pass, I would prefer (mild preference) of keeping the way things were, i.e. an op lowering (op_.py) would inject an op when needed.
This is mainly to keep things together logically and keep the tosa graph 'sound' after each op lowering.
That said I understand the code duplication aspect, but that could be tackled with keeping these insert rescale
as util functions.
Again, not a strong objection. Stamping to unblock you, and leaving it up to you to decide.
Thanks!
Some TOSA ops do not support INT8 as inputs and outputs. Instead, only INT32 is supported as a whole number type. Prior to this patch, affected node visitors inserted rescale ops between the data types INT8 and INT32 before and after the operator such that it will accept its input and output. Change this by moving the insertion of the rescale ops to a new pass called InsertRescaleInt32Pass. This will further enable optimizations to the graph by fusing the rescale nodes. Only comparison operators are handled in this patch; the remaining ones are left out to be done in another patch. Signed-off-by: Martin Lindström <Martin.Lindstroem@arm.com> Change-Id: I6bb8a10a0b453ae9fd8b8604d64cc5103a4da050
Signed-off-by: Martin Lindström <Martin.Lindstroem@arm.com> Co-authored-by: Oscar Andersson <Oscar.Andersson@arm.com> Change-Id: I62fdc5bea75361d6c32711968bdc1c9d03677ccc
3d35156
to
a647bc3
Compare
Signed-off-by: Martin Lindström <Martin.Lindstroem@arm.com> Change-Id: I5ee8f97590ce599a9dfc60ced0775654fa565c4e
Signed-off-by: Martin Lindstroem <Martin.Lindstroem@arm.com> Co-authored-by: Oscar Andersson <Oscar.Andersson@arm.com> Change-Id: I38d63015e03e59c267338c84d64731b050854d06
This reverts commit f21cf7f.
Some TOSA ops do not support INT8 as inputs and outputs. Instead, only INT32 is supported as a whole number type. Prior to this patch, affected node visitors inserted rescale ops between the data types INT8 and INT32 before and after the operator such that it will accept its input and output.
Change this by moving the insertion of the rescale ops to a new pass called
InsertRescaleInt32Pass
. This will further enable optimizations to the graph by fusing the rescale nodes.Only comparison, ABS, MAXIMUM and MINIMUM operators are handled in this patch; the remaining ones are left out to be done in another patch.
Test plan
This is refactoring which means that external behavior is not altered. A new pass
InsertRescaleInt32Pass
has been added and it comes with a new unit test in backends/arm/test/passes/test_insert_rescale_i32_pass.py.cc @digantdesai @freddan80 @per @zingo @oscarandersson8218