Arm backend: Annotate ADD/SUB with indepenedent observers #13516
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13516
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 2 New Failures, 1 Unrelated FailureAs of commit 87ac448 with merge base 71a7806 ( 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. |
meta-cla
bot
added
the
CLA Signed
gggekov
added
partner: arm
zingo
added
release notes: arm
|
@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D80562110. |
backends/arm/operators/op_add.py
Outdated
| # pyre-ignore | ||
| tqutils.insert_rescale_op_to_int8( | ||
| tosa_graph, add_output, scale_back, node, self.tosa_spec | ||
| tosa_graph, add_output, scale_back, node, False, self.tosa_spec |
There was a problem hiding this comment.
Readability nit
| tosa_graph, add_output, scale_back, node, False, self.tosa_spec | |
| tosa_graph, add_output, scale_back, node, compute_rescale=False, self.tosa_spec |
There was a problem hiding this comment.
Will do, thanks. We have a long weekend in the UK, will be back on Tuesday.
| } | ||
| fvp_sub2_xfails = {"rand_4D_2x2x4x4": "MLETORCH-517 : Multiple batches not supported"} | ||
|
|
||
| # Sub and tan - the tan has a really steep curve just before Pi/2 and a point of discontinuity at Pi/2 |
backends/arm/tosa_quant_utils.py
Outdated
| from tosa.RoundingMode import RoundingMode # type: ignore | ||
|
|
||
|
|
||
| def insert_rescale_ops_to_int32_for_add_sub( |
There was a problem hiding this comment.
Nit why the fn name should have _for_add_sub suffix?
There was a problem hiding this comment.
The insert_rescale_ops_to_int32_for_add_sub function is only called for the ADD & SUB ops because only for these operators we use a common scale of 2max(scale_A,scale_B) and then multiply the original scale by 1<<20 without overflowing in a 32-bit accumualator.
There was a problem hiding this comment.
I mean the function name shouldn't list its call sites :)
gggekov
force-pushed
the
Meta_fusing_conv1d_relu_residual_add
branch
6 times, most recently
from
80e0d2b to
03d88e1
Compare
We were annotating the ADD/SUB with a shared observer resulting in the same quantisation parameters on the two inputs even if we were adding numbers in different ranges(positive tensor to a tensor with positive and negative values). As a result, the quantisation parameters were suboptimal. This change annotates the operator with independent observers and changes how we rescale the two inputs to bring them to the same range. Added a unit test of a resnet model. Lowered the number of channels on a few unit tests in order to keep the Total SRAM Used below 2MB for the Ethos-U55 to fit within the memory limit of the Corstone-300. Change-Id: I7adde636f901c9df6b779d946a157e66fd12e24e
gggekov
force-pushed
the
Meta_fusing_conv1d_relu_residual_add
branch
from
03d88e1 to
c87751e
Compare
|
Rebased after a fix for some broken arm tests was merged |
|
Test fails are unrelated |
|
@digantdesai I cant merge this, is there a older version of this PR internally blocking this? |
|
@digantdesai has imported this pull request. If you are a Meta employee, you can view this in D80562110. |
4 participants
We were annotating the ADD/SUB with a shared observer resulting in the same quantisation parameters on the two inputs even if we were adding numbers in different ranges(positive tensor to a tensor with positive and negative values). As a result,
the quantisation parameters were suboptimal. This change annotates the operator with independent observers and changes how we rescale the two inputs to bring them to the same range. Added a unit test of a resnet model. Lowered the number of channels on a few unit tests in order to keep the Total SRAM Used below 2MB for the Ethos-U55 to fit within the memory limit of the Corstone-300.
Fixes #12959
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218