Fix INT8 quantization Triton kernel for HIP/ROCm#58
Conversation
|
🎉 Thank you for your contribution, we really appreciate it! 🎉 Like many open source projects, we require contributors to sign our Contributor License Agreement (CLA). A CLA makes the ownership of contributions explicit, so contributors and the project share a clear understanding of how the code can be used. By signing, you:
CLAs are standard practice across major open source projects including those under the Apache Software Foundation and the Linux Foundation. Ours is based on the Apache Software Foundation's CLA. Most importantly, it would enable us to relicense the project under a more permissive license in the future, giving the project and its community greater flexibility. ✍ To sign, please post a new comment on this PR with exactly the following text: ✍ I have read and agree to the Contributor License Agreement You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Warning Review limit reached
More reviews will be available in 38 minutes and 59 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesINT8 rounding change
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I have read and agree to the Contributor License Agreement. |
Replace libdevice.rint() with manual sign-aware rounding since Triton 3.6.0 HIP backend lacks rint, math.round, and int clamp. All 33 INT8 tests pass on gfx1100 (RDNA 3 / RX 7900 XT).
8cffa89 to
fe31d4b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_kitchen/backends/triton/quantization.py`:
- Around line 805-812: The rounding logic in the quantization path uses a
sign-based bias, which changes tie handling away from the previous nearest-even
behavior. Update the rounding in the relevant Triton quantization routine (the
block that computes rounding_bias and q_i) so exact .5 cases still round
ties-to-even like rint, or otherwise make the new rounding contract explicit and
add coverage for it. Verify the fix against the quantization conversion path
before the final int32 cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed7287e6-6b0c-4147-a7bb-192efbac7fd2
📒 Files selected for processing (1)
comfy_kitchen/backends/triton/quantization.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_kitchen/backends/triton/quantization.py`:
- Around line 803-817: The half-to-even tie handling in the quantization
rounding path is incorrect for negative .5 values because `floor_val % 2.0` in
the Triton/HIP path can be negative, so `q_i` in this rounding block
misclassifies odd negative floors. Update the parity check in the `tl.where`
logic to use a sign-safe even/odd test for `floor_val` so ties like -2.5 round
to the nearest even value, and add a test alongside this quantization code that
explicitly covers negative half-integers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d4016d49-1a8a-4806-b633-e5277d01f596
📒 Files selected for processing (1)
comfy_kitchen/backends/triton/quantization.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_kitchen/backends/triton/quantization.py`:
- Around line 813-815: The tie-breaking branch in _quantize_rowwise_kernel uses
is_odd before it is defined, so move or compute is_odd earlier in the kernel
before the frac == 0.5 path and ensure it is in scope when building q_i. Keep
the existing rounding logic intact, just make sure the local symbol is
initialized before the tl.where call that references floor_val + is_odd.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63dd8e08-57d2-45be-a23d-a9705bc0fccc
📒 Files selected for processing (1)
comfy_kitchen/backends/triton/quantization.py
|
gives black output specially on krea on most tries, same as my simpler version of this here #54 , custom node still has better performance |
|
I noticed the change to 3.7 change to Triton. |
3 participants
Replace libdevice.rint() with manual sign-aware rounding since Triton 3.6.0 HIP backend lacks rint, math.round, and int clamp. All 33 INT8 tests pass on gfx1100 (RDNA 3 / RX 7900 XT).
A lot of them were failing before and I was getting seg faults when trying to run workflows and started comfy with --enable-triton-backend.