Arm backend: Add support for sigmoid and tanh int16x8 #15101
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15101
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New FailuresAs of commit 007678f with merge base 1f114f1 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
|
@pytorchbot label ciflow/trunk |
|
@pytorchbot label 'partner: arm' |
pytorch-bot
bot
added
the
partner: arm
|
@pytorchbot label "release notes: none" |
pytorch-bot
bot
added
the
release notes: none
| quantizer.set_global( | ||
| get_symmetric_a16w8_quantization_config(is_per_channel=per_channel_quantization) | ||
| get_symmetric_a16w8_quantization_config( | ||
| is_per_channel=per_channel_quantization, epsilon=2**-16 |
There was a problem hiding this comment.
How does this change impact other ops?
There was a problem hiding this comment.
Thanks for review!
There could be other Table operators that may need this change I will update those as I go through all operators. But, this change will not impact others as its stands because its only set in the unit tests.
There was a problem hiding this comment.
It's set in unit tests so that the sig/tanh can be partitioned to U55/U85, right?
There was a problem hiding this comment.
As mentioned below, this is just to ensure we get the correct values on the output.
Thanks for review!
| @common.XfailIfNoCorstone300 | ||
| @pytest.mark.xfail( | ||
| reason="Vela compilation fails with 'Invalid arguments' for int16 sigmoid operations" | ||
| reason="MLETORCH-707: AssertionError: Output 0 does not match reference output." |
There was a problem hiding this comment.
This indicates we don't yet have full support for U55. Can you comment on what's remaining? cc: @3l1
There was a problem hiding this comment.
This is more so a limitation in terms of U55 support and not a int16x8 specific issue.
Thanks for review!
| quantizer.set_global( | ||
| get_symmetric_a16w8_quantization_config(is_per_channel=per_channel_quantization) | ||
| get_symmetric_a16w8_quantization_config( | ||
| is_per_channel=per_channel_quantization, epsilon=2**-16 |
There was a problem hiding this comment.
It's set in unit tests so that the sig/tanh can be partitioned to U55/U85, right?
| is_dynamic: bool = False, | ||
| weight_qmin: int = -127, | ||
| weight_qmax: int = 127, | ||
| epsilon: float = 2**-12, |
There was a problem hiding this comment.
Ok so in order for sig/tanh to be able to partition, we need to set this to 2**-16 right?
There was a problem hiding this comment.
It can be partitioned without it, this is more so for numerical behavior. If the set epsilon is too high, the quantization arguments can be inflated resulting in an incorrect output.
Thank you for review!
3 participants
Summary
Adds support for sigmoid and tanh to arm backend for int16x8 support
Removes unnecessary test_sigmoid_16bit as these are now covered by test_sigmoid
cc @freddan80 @per @zingo @oscarandersson8218 @digantdesai