Fix portable dequantize_per_channel misreading Int zero points - #21773
Open
karan1508 wants to merge 1 commit into
Open
Fix portable dequantize_per_channel misreading Int zero points#21773karan1508 wants to merge 1 commit into
karan1508 wants to merge 1 commit into
Conversation
Summary:
## Why?
A quantized tensor stores small integers plus two numbers that convert them
back to real values:
real_value = (stored_integer - zero_point) x scale
"Per-channel" means each output channel gets its own zero point and scale, so
those are little arrays - one entry per channel.
The zero-point array is allowed to be stored as either 32-bit or 64-bit
integers. The dequantization operator checked for exactly that and accepted
both, and then read the array as if it were always 64-bit. So whenever the
array really was 32-bit, the operator read memory at the wrong stride and two
things went wrong at once.
**It glued neighbouring channels together.** Say the real zero points are
[30, 50, 60, 90] stored as 32-bit. Reading them as 64-bit pairs them up:
channel 0 -> 30 + 50 x 2^32 ~ 214,748,364,830
channel 1 -> 60 + 90 x 2^32 ~ 386,547,056,700
**It ran off the end.** Four 64-bit reads need 32 bytes, but a 4-entry 32-bit
array only holds 16. Channels 2 and 3 read whatever memory happened to sit
after the array - random garbage.
Plug a nonsense zero point into the formula and everything downstream is wrong:
(bias - 214,748,364,830) x scale -> 78.7 instead of 0.047
The giveaway while debugging was an all-zero bias dequantizing to 37.7. Zero
minus garbage isn't zero.
Weights were never affected - their zero-point arrays happen to be stored as
64-bit. Biases are the ones stored as 32-bit, so every model with quantized
biases was hitting this. It only became visible in deep models, where one bad
bias per layer compounds through ~50 stacked convolutions until the activations
slam into the int16 ceiling and the output is pure noise. Shallower models
stayed inside their test tolerances and looked fine, which is why this went
unnoticed for so long.
## What?
Read the array at whatever width it actually is. The helper sits next to the
existing one for the scale array, which already did exactly this - the
zero-point path had just never been given the same treatment:
int64_t get_zero_point(const Tensor& zero_points, size_t channel_ix) {
if (zero_points.scalar_type() == ScalarType::Int) {
return static_cast<int64_t>(
zero_points.const_data_ptr<int32_t>()[channel_ix]); // 32-bit
}
return zero_points.const_data_ptr<int64_t>()[channel_ix]; // 64-bit
}
Both places that used to hardcode the 64-bit read now call it, so the general
and optimized code paths can no longer disagree. That is the entire fix - no
logic change, just reading the right number of bytes.
The new regression test covers the 32-bit width across input types and both the
single- and multi-dimensional shapes. Every pre-existing test for this operator
happened to build its zero-point array as 64-bit, so the accepted-but-broken
case was never once exercised.
Reviewed By: digantdesai
Differential Revision: D115185783
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21773
Note: Links to docs will display an error until the docs builds have been completed. ❌ 6 Awaiting Approval, 2 New FailuresAs of commit 7bb6c2e with merge base d189b45 ( AWAITING APPROVAL - The following workflows need approval before CI can run:
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
Contributor
|
@karan1508 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115185783. |
digantdesai
approved these changes
Aug 12, 2026
digantdesai
left a comment
digantdesai
left a comment
Contributor
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
This PR needs a
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Why?
A quantized tensor stores small integers plus two numbers that convert them
back to real values:
"Per-channel" means each output channel gets its own zero point and scale, so
those are little arrays - one entry per channel.
The zero-point array is allowed to be stored as either 32-bit or 64-bit
integers. The dequantization operator checked for exactly that and accepted
both, and then read the array as if it were always 64-bit. So whenever the
array really was 32-bit, the operator read memory at the wrong stride and two
things went wrong at once.
It glued neighbouring channels together. Say the real zero points are
[30, 50, 60, 90] stored as 32-bit. Reading them as 64-bit pairs them up:
It ran off the end. Four 64-bit reads need 32 bytes, but a 4-entry 32-bit
array only holds 16. Channels 2 and 3 read whatever memory happened to sit
after the array - random garbage.
Plug a nonsense zero point into the formula and everything downstream is wrong:
The giveaway while debugging was an all-zero bias dequantizing to 37.7. Zero
minus garbage isn't zero.
Weights were never affected - their zero-point arrays happen to be stored as
64-bit. Biases are the ones stored as 32-bit, so every model with quantized
biases was hitting this. It only became visible in deep models, where one bad
bias per layer compounds through ~50 stacked convolutions until the activations
slam into the int16 ceiling and the output is pure noise. Shallower models
stayed inside their test tolerances and looked fine, which is why this went
unnoticed for so long.
What?
Read the array at whatever width it actually is. The helper sits next to the
existing one for the scale array, which already did exactly this - the
zero-point path had just never been given the same treatment:
Both places that used to hardcode the 64-bit read now call it, so the general
and optimized code paths can no longer disagree. That is the entire fix - no
logic change, just reading the right number of bytes.
The new regression test covers the 32-bit width across input types and both the
single- and multi-dimensional shapes. Every pre-existing test for this operator
happened to build its zero-point array as 64-bit, so the accepted-but-broken
case was never once exercised.
Reviewed By: digantdesai
Differential Revision: D115185783