Skip to content

[CPU] Fix output saturation in int8 QLinearSoftmax - #29728

Merged
hariharans29 merged 3 commits into
microsoft:mainfrom
mcollinswisc:fix_qlinearsoftmax_saturation
Jul 17, 2026
Merged

[CPU] Fix output saturation in int8 QLinearSoftmax#29728
hariharans29 merged 3 commits into
microsoft:mainfrom
mcollinswisc:fix_qlinearsoftmax_saturation

Conversation

@mcollinswisc

Copy link
Copy Markdown
Contributor

Description

Uses the correct int8 range to clamp/saturate the output of the int8 specialization of the QLinearSoftmax kernel.
Previously there was a copy-paste error, and it was saturating to 255 like in the uint8 specialization.

https://github.com/microsoft/onnxruntime/blob/main/docs/ContribOperators.md#com.microsoft.QLinearSoftmax

This change also adds a unit test case that causes output saturation, and reproduced the bug.

Motivation and Context

When there are:

  1. Softmax outputs very close to 1 (in the original float graph), and
  2. a y_scale of 1.0 / 256.0 and y_zero_point of -128 (as set by https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/python/tools/quantization/operators/softmax.py)

then the output of int8 QLinearSoftmax will saturate. Incorrect clamping is causing this to wrap around to -128 instead, producing incorrect results.

#29727

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@hariharans29
hariharans29 requested a review from Copilot July 16, 2026 01:26
@hariharans29 hariharans29 changed the title Fix output saturation in int8 QLinearSoftmax [CPU] Fix output saturation in int8 QLinearSoftmax Jul 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes incorrect output clamping in the CPU contrib com.microsoft.QLinearSoftmax kernel for int8 outputs, preventing values that should saturate at 127 from instead wrapping around to -128. It also adds a regression unit test that reproduces the saturation scenario reported in #29727.

Changes:

  • Fix int8 output saturation in QlinearSoftmaxCPU<int8_t> by clamping to the proper int8 range ([-128, 127]) instead of using the uint8-style 255 upper bound.
  • Add a unit test that exercises the saturation edge case with Y_scale=1/256 and Y_zero_point=-128.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
onnxruntime/contrib_ops/cpu/quantization/qlinear_softmax.cc Corrects int8 output clamping to saturate to int8_t min/max instead of uint8 max.
onnxruntime/test/contrib_ops/qlinear_lookup_table_test.cc Adds a regression test to validate correct saturation behavior for int8 QLinearSoftmax.

Comment thread onnxruntime/test/contrib_ops/qlinear_lookup_table_test.cc Outdated
@hariharans29

Copy link
Copy Markdown
Member

Review: PR #29728[CPU] Fix output saturation in int8 QLinearSoftmax

Verdict — approve pending CI green on adf13a7. Minimal, well-scoped bug fix + faithful regression test. The root cause is a clean int8-vs-uint8 copy-paste error at qlinear_softmax.cc line 246 pre-patch, the fix is idiomatic C++17, and the test reproduces the exact scenario the quantizer produces per onnxruntime/python/tools/quantization/operators/softmax.py. 2 files, +40/−1, 3 commits.

What looks correct

  1. Root cause is precisely identified. The buggy line was:

    const int8_t vy = static_cast<int32_t>(vq) > 255 ? static_cast<int8_t>(255) : static_cast<int8_t>(vq);

    This is the uint8 clamp shape (255 upper bound, no lower bound) applied to the int8_t output. Two defects at once:

    • No upper-bound clamp for int8: any vq in [128, 255] sails through the > 255 check and gets narrowed to int8_t, producing a wrap-around to [-128, -1]. This is exactly the failure the reporter saw at vq = 128 (softmax ≈ 1.0, Y_scale = 1/256, Y_zero_point = -128vq = round(1.0 * 256) + (-128) = 128, wraps to -128).
    • No lower-bound clamp either: any vq < -128 would wrap to positive. Not reachable in typical softmax (both terms in vq = round(softmax / Y_scale) + Y_zero_point are nonneg for standard configs), but the old code was silently unsafe here as well.
  2. Fix is idiomatic:

    constexpr int32_t low  = std::numeric_limits<int8_t>::lowest();  // -128
    constexpr int32_t high = std::numeric_limits<int8_t>::max();     //  127
    ...
    const int8_t vy = static_cast<int8_t>(std::clamp(vq, low, high));

    std::clamp with numeric_limits<int8_t> bounds is exactly the right shape — covers both directions, self-documenting, and drops the redundant static_cast<int32_t>(vq) from the old expression (vq was already int32_t). New #include <algorithm> and #include <limits> are correct and minimal.

  3. Uint8 sibling QlinearSoftmaxCPU<uint8_t> does not have a mirror defect, so this fix is correctly scoped to the int8_t specialization only. In the uint8 path, vq is uint32_t (can't underflow) and the upper bound 255 is genuinely numeric_limits<uint8_t>::max(). No change needed there.

  4. Regression test in test/contrib_ops/qlinear_lookup_table_test.cc faithfully reproduces the failure.

    • X = {127, -128, -128, -128} with X_scale = 0.1, X_zero_point = 0 dequantizes to {12.7, -12.8, -12.8, -12.8}. The dominant first element makes softmax[0] ≈ 1.0 (the other three are ~0 to fp32 precision).
    • Y_scale = 1/256, Y_zero_point = -128 — the exact pair python/tools/quantization/operators/softmax.py produces per the test's inline comment.
    • Requantizing softmax[0] ≈ 1.0: vq = round(1.0 / (1/256)) + (-128) = 128, needs to saturate to 127.
    • Expected Y = {127, -128, -128, -128}. Pre-patch, Y[0] would wrap to -128; the test would fail on main. Post-patch, it passes. Good regression coverage.
  5. Rounding-mode discipline. Test brackets test.Run() with origin_round_mode = std::fegetround(); std::fesetround(FE_TONEAREST); ... std::fesetround(origin_round_mode); because the kernel uses std::nearbyintf, which is rounding-mode-sensitive. Correct save/restore around the assertion. Matches the neighboring QLinearSoftmax_Int8_v12 test's shape.

  6. Both add_shape_to_input = true/false runs are exercised, matching the sibling test's style — covers both the dynamic-shape and static-shape input paths through the op tester.

  7. The Copilot review nit was addressed in commit adf13a7 — the Y_scale / Y_zero_point comment was reworded to point explicitly at onnxruntime/python/tools/quantization/operators/softmax.py as the source of those specific values. This makes the test self-documenting about why it picks those numbers rather than reading like magic.

Non-blocking observations

  1. Nothing in this PR touches the lookup table. The bug lives strictly in the post-LUT re-quant step, and the LUT itself already produces values in-range. Scope of the fix matches scope of the bug.

  2. Consider a matching underflow-case test. As noted, no realistic softmax input can produce vq < -128 under the standard y_scale, y_zero_point configurations, so this is defensive-only. But since the new std::clamp covers the lower bound too, a second test case that explicitly forces vq into the underflow region (say Y_zero_point = 127 with a softmax output near 0) would document that the fix is bidirectional. Not required.

  3. The test comment on line 307–308 is well-worded (First element expected to saturate to 127 (would otherwise mathematically be 128)) — future readers won't wonder why X[0] = 127 maps to Y[0] = 127 rather than something quantitatively different.

CI

Latest commit adf13a7 shows 1/68 checks OK (still catching up — matches the microsoft-side CI infrastructure lag other PRs are seeing this week). Prior commit cbdf81e reached 71/86 without regressions on the pipelines that ran. Since adf13a7 is comment-only (Copilot suggestion applied), no CI regression is expected. Recommend confirming a full pipeline pass on adf13a7 before merge; nothing about this fix touches EP-specific code, so any x86 / ARM64 CPU CI leg is sufficient signal.

Bottom line

Textbook signed-vs-unsigned copy-paste bugfix. The root-cause explanation in the PR description is correct, the fix is the right shape, and the regression test exactly matches the reported symptom in #29727 using the same Y_scale / Y_zero_point pair the ORT quantizer produces for softmax outputs. Ready to land once CI settles on adf13a7.

@hariharans29
hariharans29 enabled auto-merge (squash) July 17, 2026 01:13
@hariharans29

Copy link
Copy Markdown
Member

Can you please fix the failing lint checks ?

@mcollinswisc

Copy link
Copy Markdown
Contributor Author

Well, the lint one looks like some kind of toolchain error.

image

It failed the Python lint check, and this PR is C++ only and does not edit any Python files. I'll try to root cause the other failures too, thanks for kicking off the CI.

@mcollinswisc

Copy link
Copy Markdown
Contributor Author

Followed up in #29750

@mcollinswisc

Copy link
Copy Markdown
Contributor Author

And following up on the "Windows x64 QNN CI Pipeline" failure in #29753

tianleiwu pushed a commit that referenced this pull request Jul 17, 2026
### Description

Copies the Java setup step from sibling files `windows_webgpu.yml`,
`windows_x64_release_xnnpack.yml`,
`windows_x64_release_build_x64_release.yml` to `windows_qnn_x64.yml`.

This explicitly ensures a compatible Java version, rather than relying
on the JDK already installed on the self-hosted runner. Otherwise there
may be breakages when switching the runner.

### Motivation and Context

I believe this will fix the failures in CI for the Windows x64 QNN CI
Pipeline. I'm seeing it fail in both an unrelated PR:

#29728

https://github.com/microsoft/onnxruntime/actions/runs/29509983398/job/87780661649?pr=29728

and in commits to main:


https://github.com/microsoft/onnxruntime/actions/runs/29546572913/job/87780090142

https://github.com/microsoft/onnxruntime/actions/runs/29499730505/job/87625355397

This is assuming the chain of causation:

1. #29731 switches the
runner pool
2. The `windows_qnn_x64.yml` pipeline finds JDK 8 already on the runner
(previous runner pool had JDK 11)
3. The Spotless Gradle plugin v7.2.1 isn't compatible with JDK 8
4. onnxruntime Java project can't be configured
5. Build fails

This new workflow action hopefully installs a compatible JDK & all will
be well.

#29753
@hariharans29

Copy link
Copy Markdown
Member

Can you please rebase ? Maybe the checks will all pass now

@hariharans29

Copy link
Copy Markdown
Member

If you do hit genuine linting errors again, here are the steps to mitigate:

This project uses [lintrunner](https://github.com/suo/lintrunner) for linting. It provides a consistent linting experience locally and in CI. You can install the dependencies and initialize with

Adds a test case that checks the output saturation behavior of QLinearSoftmax.

There's currently a bug:

https://gist.github.com/mcollinswisc/b3c9a2d9cf50b7bf289ced1d6be2c709

and this test is expected to fail with message:

[ RUN      ] QLinearLookupTableBasedOperatorTests.QLinearSoftmax_Int8_Saturate
/mnt/themis/artevelde/research/onnxruntime/onnxruntime/test/unittest_util/checkers.cc:393: Failure
Expected equality of these values:
  cur_expected[i]
    Which is: '\x7F' (127)
  cur_actual[i]
    Which is: '\x80' (-128)
i:0
Old code copied the saturation from the uint8 kernel, which caused int8
outputs to wrap instead.
auto-merge was automatically disabled July 17, 2026 19:41

Head branch was pushed to by a user without write access

@mcollinswisc
mcollinswisc force-pushed the fix_qlinearsoftmax_saturation branch from adf13a7 to dfbf9c7 Compare July 17, 2026 19:41
@mcollinswisc

Copy link
Copy Markdown
Contributor Author

Can you please rebase ? Maybe the checks will all pass now

Certainly, I have rebased it

@hariharans29
hariharans29 enabled auto-merge (squash) July 17, 2026 20:51
@hariharans29
hariharans29 merged commit 8585c64 into microsoft:main Jul 17, 2026
116 of 200 checks passed
tianleiwu pushed a commit that referenced this pull request Jul 17, 2026
Copies the Java setup step from sibling files `windows_webgpu.yml`,
`windows_x64_release_xnnpack.yml`,
`windows_x64_release_build_x64_release.yml` to `windows_qnn_x64.yml`.

This explicitly ensures a compatible Java version, rather than relying
on the JDK already installed on the self-hosted runner. Otherwise there
may be breakages when switching the runner.

I believe this will fix the failures in CI for the Windows x64 QNN CI
Pipeline. I'm seeing it fail in both an unrelated PR:

#29728

https://github.com/microsoft/onnxruntime/actions/runs/29509983398/job/87780661649?pr=29728

and in commits to main:

https://github.com/microsoft/onnxruntime/actions/runs/29546572913/job/87780090142

https://github.com/microsoft/onnxruntime/actions/runs/29499730505/job/87625355397

This is assuming the chain of causation:

1. #29731 switches the
runner pool
2. The `windows_qnn_x64.yml` pipeline finds JDK 8 already on the runner
(previous runner pool had JDK 11)
3. The Spotless Gradle plugin v7.2.1 isn't compatible with JDK 8
4. onnxruntime Java project can't be configured
5. Build fails

This new workflow action hopefully installs a compatible JDK & all will
be well.

#29753
@mcollinswisc
mcollinswisc deleted the fix_qlinearsoftmax_saturation branch July 18, 2026 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants