Comfy-Org/comfy-kitchen

Fork 0

mirror of https://github.com/Comfy-Org/comfy-kitchen.git synced 2026-11-04 23:37:42 +00:00

[PR #93] [MERGED] HIP port of #88: swiglu input_act and partial-rotary rot_dim #139

New issue

Closed

opened 2026-10-26 22:20:45 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:20:45 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-kitchen/pull/93
Author: @0xDELUXA
Created: 8/4/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @comfyanonymous

Base: main ← Head: amd/hip-swiglu-rotdim

📝 Commits (1)

b07fb07 HIP port of #88: swiglu input_act and partial-rotary rot_dim

📊 Changes

6 files changed (+319 additions, -56 deletions)

View changed files

📝 comfy_kitchen/backends/hip/__init__.py (+34 -28)
📝 comfy_kitchen/backends/hip/dlpack_bindings.cpp (+27 -15)
📝 comfy_kitchen/backends/hip/hadamard.h (+25 -7)
📝 comfy_kitchen/backends/hip/ops/quantize_int8.hip (+6 -1)
📝 comfy_kitchen/backends/hip/ops/rms_rope.hip (+24 -4)
📝 tests/test_hip_wmma.py (+203 -1)

📄 Description

Ports #88 to the HIP backend.

#88 added swiglu as an input_act and a rot_dim prefix for rms_rope_split_half, fusing both into the CUDA kernels. HIP took them as eager fallbacks, so results were already correct. This is a performance port, not a correctness fix.

Partial rotary `rot_dim`

rms_rope.hip rotates the first rot_dim dims in split-half pairs (p, p + rot_dim/2) and adds a norm-only tail for the rest. The RMS reduction still spans the full head_dim. The tail's store performs the single rounding eager's rms_norm applies, so no extra rounding step is needed there.

require_rope_shapes now validates the freqs table against rot_dim instead of head_dim. It is shared with apply_rope, which passes head_dim and is unchanged. rot_dim = 0 keeps rotating everything.

The eager bail-outs in rms_rope_split_half / rms_rope_split_half_ are gone. The GQA path, where q and k have different head counts and route through single-tensor calls, carries rot_dim through as well.

SwiGLU `input_act`

The ConvRot int8 quantizer gains act code 2, reading a [gate | up] row twice as wide as the K it stages and writes. Only the activated row is staged in LDS, so the per-device K budget is unchanged.

int8_linear now tracks the activated width k_act separately from the raw input width, so the % 16 check, the group-divisibility check and the LDS budget all see the width actually being quantized rather than the doubled input. Routes
the fused quantizer cannot serve (no convrot, K over the LDS cap) still apply the activation eagerly, matching the CUDA backend.

Architecture coverage

rms_rope.hip has no WMMA dependency, so partial rotary works on RDNA2 through RDNA4. rms_rope_split_half is not in _WMMA_ONLY_OPS, so it still dispatches to HIP on RDNA2.

The swiglu kernel also compiles everywhere, but on RDNA2 int8_linear is dropped from the constraints (no matrix cores), so the fused path is unreachable there and the op routes to triton/eager. Same situation as the existing gelu_tanh fusion.

Compile-verified on gfx1030 (RDNA2), gfx1100 (RDNA3), gfx1151 (RDNA3.5) and gfx1200 (RDNA4). Run-verified on RDNA4 only - the test machine has a single gfx1200, which cannot execute gfx11 or gfx10 code.

Tests

14 new cases in tests/test_hip_wmma.py:

partial rotary against a reference across rot_dim and dtype
rot_dim == head_dim bit-identical to omitting it
in-place on packed-QKV slices, with v proven untouched
odd rot_dim rejected
swiglu on all three routes (fused convrot, no convrot, m == 1)
fused swiglu at least as accurate as materializing silu(gate) * up first

#88's tests parametrize over ["cuda", "triton", "eager"] only, so HIP coverage has to live in the HIP suite.

One existing line changed: test_rms_rope_rejects_a_short_weight calls the raw _C.rms_rope binding positionally and needed the new argument.

Results

test_hip_wmma.py, test_hip_dispatch.py, test_int8_input_act.py and test_int8.py: 335 passed, 64 skipped.

Full suite on gfx1200 (ROCm 7.15, torch 2.12): the failure set is identical with and without this change, verified by stashing and re-running. Those failures are pre-existing NVFP4/MXFP8 and triton-tolerance cases unrelated to the HIP backend.

Measured on gfx1200 at MiniMax shapes (56 x 128 heads, rot_dim 96, ffn 14336), against the eager fallback this replaces:

	S=4096	S=8192
`rms_rope_split_half_`	10.05 -> 2.06 ms (4.9x)	20.19 -> 3.99 ms (5.1x)
`int8_linear` (fc2)	12.84 -> 11.77 ms (1.09x)	26.19 -> 24.36 ms (1.08x)

Across 50 DiT blocks that is roughly 0.9 s per denoise step. MiniMax (comfy/ldm/minimax/model.py) is currently the only ComfyUI model requesting either feature, so this is a no-op elsewhere.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-kitchen/pull/93 **Author:** [@0xDELUXA](https://github.com/0xDELUXA) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@comfyanonymous](https://github.com/comfyanonymous) **Base:** `main` ← **Head:** `amd/hip-swiglu-rotdim` --- ### 📝 Commits (1) - [`b07fb07`](https://github.com/Comfy-Org/comfy-kitchen/commit/b07fb07ba57fad4670cd3facdd06cb8341f1c057) HIP port of #88: swiglu input_act and partial-rotary rot_dim ### 📊 Changes **6 files changed** (+319 additions, -56 deletions) <details> <summary>View changed files</summary> 📝 `comfy_kitchen/backends/hip/__init__.py` (+34 -28) 📝 `comfy_kitchen/backends/hip/dlpack_bindings.cpp` (+27 -15) 📝 `comfy_kitchen/backends/hip/hadamard.h` (+25 -7) 📝 `comfy_kitchen/backends/hip/ops/quantize_int8.hip` (+6 -1) 📝 `comfy_kitchen/backends/hip/ops/rms_rope.hip` (+24 -4) 📝 `tests/test_hip_wmma.py` (+203 -1) </details> ### 📄 Description Ports #88 to the HIP backend. #88 added `swiglu` as an `input_act` and a `rot_dim` prefix for `rms_rope_split_half`, fusing both into the CUDA kernels. HIP took them as eager fallbacks, so results were already correct. This is a performance port, not a correctness fix. ### Partial rotary `rot_dim` `rms_rope.hip` rotates the first `rot_dim` dims in split-half pairs `(p, p + rot_dim/2)` and adds a norm-only tail for the rest. The RMS reduction still spans the full `head_dim`. The tail's store performs the single rounding eager's `rms_norm` applies, so no extra rounding step is needed there. `require_rope_shapes` now validates the freqs table against `rot_dim` instead of `head_dim`. It is shared with `apply_rope`, which passes `head_dim` and is unchanged. `rot_dim = 0` keeps rotating everything. The eager bail-outs in `rms_rope_split_half` / `rms_rope_split_half_` are gone. The GQA path, where q and k have different head counts and route through single-tensor calls, carries `rot_dim` through as well. ### SwiGLU `input_act` The ConvRot int8 quantizer gains act code 2, reading a `[gate | up]` row twice as wide as the K it stages and writes. Only the activated row is staged in LDS, so the per-device K budget is unchanged. `int8_linear` now tracks the activated width `k_act` separately from the raw input width, so the `% 16` check, the group-divisibility check and the LDS budget all see the width actually being quantized rather than the doubled input. Routes the fused quantizer cannot serve (no convrot, K over the LDS cap) still apply the activation eagerly, matching the CUDA backend. ### Architecture coverage `rms_rope.hip` has no WMMA dependency, so partial rotary works on RDNA2 through RDNA4. `rms_rope_split_half` is not in `_WMMA_ONLY_OPS`, so it still dispatches to HIP on RDNA2. The swiglu kernel also compiles everywhere, but on RDNA2 `int8_linear` is dropped from the constraints (no matrix cores), so the fused path is unreachable there and the op routes to triton/eager. Same situation as the existing `gelu_tanh` fusion. Compile-verified on gfx1030 (RDNA2), gfx1100 (RDNA3), gfx1151 (RDNA3.5) and gfx1200 (RDNA4). Run-verified on RDNA4 only - the test machine has a single gfx1200, which cannot execute gfx11 or gfx10 code. ### Tests 14 new cases in `tests/test_hip_wmma.py`: - partial rotary against a reference across `rot_dim` and dtype - `rot_dim == head_dim` bit-identical to omitting it - in-place on packed-QKV slices, with `v` proven untouched - odd `rot_dim` rejected - swiglu on all three routes (fused convrot, no convrot, m == 1) - fused swiglu at least as accurate as materializing `silu(gate) * up` first #88's tests parametrize over `["cuda", "triton", "eager"]` only, so HIP coverage has to live in the HIP suite. One existing line changed: `test_rms_rope_rejects_a_short_weight` calls the raw `_C.rms_rope` binding positionally and needed the new argument. ### Results `test_hip_wmma.py`, `test_hip_dispatch.py`, `test_int8_input_act.py` and `test_int8.py`: 335 passed, 64 skipped. Full suite on gfx1200 (ROCm 7.15, torch 2.12): the failure set is identical with and without this change, verified by stashing and re-running. Those failures are pre-existing NVFP4/MXFP8 and triton-tolerance cases unrelated to the HIP backend. Measured on gfx1200 at MiniMax shapes (56 x 128 heads, `rot_dim` 96, ffn 14336), against the eager fallback this replaces: | | S=4096 | S=8192 | |---|---|---| | `rms_rope_split_half_` | 10.05 -> 2.06 ms (4.9x) | 20.19 -> 3.99 ms (5.1x) | | `int8_linear` (fc2) | 12.84 -> 11.77 ms (1.09x) | 26.19 -> 24.36 ms (1.08x) | Across 50 DiT blocks that is roughly 0.9 s per denoise step. MiniMax (`comfy/ldm/minimax/model.py`) is currently the only ComfyUI model requesting either feature, so this is a no-op elsewhere. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:20:45 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-26 22:21:05 +00:00

[PR #139] [ROCm] Add gfx12 INT8 WMMA GEMM schedule and fused INT8 ops for Z-Image #167

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/comfy-kitchen#139

No description provided.

Rows
Columns