Skip to content

Fix clipped QDQ scale gradients with nonzero zero_point_shift - #4118

Closed
cherry77-cloud (taking-lying-flat) wants to merge 1 commit into
qualcomm:developfrom
taking-lying-flat:fix/qdq-zero-point-shift-scale-grad
Closed

cherry77-cloud (taking-lying-flat) wants to merge 1 commit into
qualcomm:developfrom
taking-lying-flat:fix/qdq-zero-point-shift-scale-grad

Conversation

@taking-lying-flat

Copy link
Copy Markdown
Contributor

Affine QDQ omits zero_point_shift from the scale gradient when an input is clipped. For x=10, scale=1, offset=-1, quantization range [0, 3], shift 0.5, and upstream gradient 5, the forward output is 2.5 but backward returns 10.0 instead of the correct 12.5.

Include the shift in the clipped branch of the PyTorch backward and all three Triton backward kernels. Add regression coverage for both clipping bounds, zero/nonzero shifts, and per-tensor/per-channel/per-block layouts, including a forward finite-difference check. The test also verifies that the Triton cases actually use the Triton autograd function, since the PyTorch fallback had the same bug.

Validation on an NVIDIA RTX A1000 Laptop GPU with PyTorch 2.13.0+cu130 and Triton 3.7.1:

  • Before the fix, the new regression has 24 failing nonzero-shift cases and 24 passing zero-shift controls. All 48 pass after the fix, including eager/compiled PyTorch forward implementations.
  • test_backend.py -k 'qdq_clipped_scale_gradient or compare_qdq_gradients_with_autograd_results or test_pgs': 104 passed.
  • test_triton.py -k test_quantize_dequantize_per_tensor: 160 passed.
  • Pre-commit hooks passed for all changed files using Python 3.12; the configured Python 3.10 interpreter was unavailable locally. The repository configuration is unchanged.

Fixes #4117.

Signed-off-by: taking-lying-flat <1615405@qq.com>
@kyunggeunlee

Copy link
Copy Markdown
Contributor

Hi cherry77-cloud (@taking-lying-flat), thank you for your finding and bug fix. This was a genuine bug, and your bug fix also looks correct. We are currently reviewing this change internally. We will let you know when this change gets merged to the mainline.

@taking-lying-flat

Copy link
Copy Markdown
Contributor Author

Hi Kyunggeun Lee (@kyunggeunlee), thank you very much for confirming the bug and reviewing my fix. AIMET is an excellent project, and I sincerely appreciate the work you and the team put into it. I am grateful for the opportunity to contribute.

May I ask a small favor? If the internal review confirms that this fix is correct and suitable for inclusion, would it be possible to merge this PR directly, rather than incorporating the fix separately and then closing the PR as happened with my earlier contributions? I appreciate that those fixes were included, and I understand the team may have workflow constraints. Still, having this PR itself merged would mean a lot to me as a contributor. I would be very happy to make any changes or add any tests needed to help with that.

Thank you again for your time, consideration, and all your work on AIMET.

@kyunggeunlee

Copy link
Copy Markdown
Contributor

cherry77-cloud (@taking-lying-flat) I completely understand your motivation. Unfortunately, though, we can’t merge this public PR directly. Like many other open-source projects, our public GitHub repository’s main branch is kept in sync with our internal repository in one direction: commits merged into the internal repository are automatically copied to the public repository, but not vice versa.

Instead, I’d like to suggest an alternative: we can clone this PR as-is into our internal repository and merge it there, while preserving the commit history so that you are listed as the sole author of the change rather than as a co-author.

This way, you’ll receive full credit for your contribution without disrupting our internal CI workflow. The public commit history will look like:
image

If this sounds good to you, I’ll go ahead and start the internal process to merge your contribution. Please let us know what you think.

@taking-lying-flat

Copy link
Copy Markdown
Contributor Author

Hi Kyunggeun Lee (@kyunggeunlee), thank you very much for explaining the workflow and offering this alternative. That sounds good to me. Please go ahead with cloning this PR into the internal repository and starting the merge process as you suggested.

I really appreciate your understanding and your willingness to preserve the commit history and my authorship. Thank you for taking the time to find a solution that works with your team's process. I'm happy to help if any further changes or tests are needed.

@kyunggeunlee

Copy link
Copy Markdown
Contributor

cherry77-cloud (@taking-lying-flat) This PR was merged in aa91641 with you listed as the sole author.

Thank you for bringing the authorship issue to our attention. We believe it’s important to give proper and fair credit to our contributors.

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.

QDQ scale gradients omit zero_point_shift for clipped inputs

2 participants