Skip to content

qwen4exp: add hc ops - #28901

Merged
am17an merged 1 commit into
masterfrom
qwen4exp-hc-fuse
Sep 16, 2026
Merged

am17an merged 1 commit into
masterfrom
qwen4exp-hc-fuse

Conversation

@am17an

@am17an am17an commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Overview

Qwen4Exp uses hyperconnection ops like DSV4 but in 2 slightly different ways:

  1. a per element gate in hc_pre
  2. a NULL comb in hc_comb

This PR adds both of them without changing ggml ops. On a DGX spark

CPU Model Test t/s qwen4exp-hc-normfix t/s qwen4exp-hc-fuse-wip Speedup
CPU qwen4exp A3B IQ1_S - 1.5625 bpw pp2048 964.36 1096.91 1.14
CPU qwen4exp A3B IQ1_S - 1.5625 bpw tg32 37.17 37.85 1.02

Additional information

Requirements

@am17an
am17an requested review from a team, CISC and ggerganov as code owners September 14, 2026 14:11
@pwilkin

pwilkin commented Sep 14, 2026

Copy link
Copy Markdown
Member

/bot review

@ggml-gh-bot

ggml-gh-bot Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Automated code review

Review: qwen4exp: add hc ops (gated hc_pre, identity-comb hc_post)

Scope: extends the existing GGML_OP_DSV4_HC_PRE / GGML_OP_DSV4_HC_POST semantics (per-element sigmoid gate + scale in PRE, comb == NULL identity in POST), CPU + CUDA implementations, correct rejection in Metal/SYCL/Vulkan, and a qwen4exp graph fast path plus test-backend-ops cases. No new ops, no public API signature changes.

Blocking

None found. I verified the math: the fused gated PRE (scale * sum_h x[i,h,t]*sigmoid(gate[i,h,t]), scale = 1/hc) and the identity POST (x[i,t]*post[dst,t] + residual[i,dst,t]) are element-for-element equivalent to the reference paths they replace in src/models/qwen4exp.cpp (same reshape layout [n_embd, hc, nt] from [hc*n_embd, nt], so the stream pairing matches the old elementwise mul). Op-param layout (f32 at 0, i32 at 1) does not overlap, op params are always set by the single _impl constructor, and RPC serializes op_params with the tensor, so remote CPU execution is correct. Backend gating is consistent: CUDA implements both variants and claims them; SYCL/Vulkan/Metal reject the variants their kernels cannot run (and the previous NULL-src[3] deref in supports_op for Metal/SYCL would have crashed on the identity POST - the added null guards fix that). The per-model resolve_fused_ops probe means the shared cparams.fused_dsv4_hc_pre/post flags are resolved against the current model's graph, so unsupported backends fall back to the reference path automatically.

Will slow the review

(point 1) Latent scale trap on SYCL/Vulkan/Metal PRE. Their kernels do not apply the new scale op param, and their supports_op checks reject only the gated variant (ggml_get_op_params_i32(op, 1) != 0). Today this is safe only because ggml_dsv4_hc_pre() hardcodes scale = 1.0f; nothing enforces the invariant "non-gated implies scale == 1". A future non-gated caller passing scale != 1 would silently compute wrong results on those three backends. Cheapest fix: also reject in supports_op when ggml_get_op_params_f32(op, 0) != 1.0f, or apply the scale in those kernels.

(point 2) docs/ops.md / docs/ops/*.csv have no entries for the DSV4_HC ops at all (pre-existing from the PR that introduced them), so the extended semantics are also undocumented. Not this PR's obligation, but worth a follow-up since the ops now have two distinct shapes/modes.

Nits

(point 3) tests/test-backend-ops.cpp: the identity POST is only exercised at (31,17) and (4096,21); the comb variant also has the (1,1) degenerate-size case. Consider adding test_dsv4_hc_post(1, 1, true) for symmetry.

(point 4) ggml/include/ggml.h around the new declaration: the trailing lone // line after the result[i, t] = ... comment - the adjacent hc_post block uses the same pattern, so it blends in, just noting it.

Otherwise the diff is clean: no unicode, concise comments, existing style followed (_impl wrapper pattern, GGML_TENSOR_LOCALS, template kernels selected via function pointer with the existing ggml_cuda_kernel_launch helper), and test cases for both new variants are included per the two-backend convention.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

@github-actions github-actions Bot added model Model specific testing Everything test related Vulkan Issues specific to the Vulkan backend ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language Apple Metal https://en.wikipedia.org/wiki/Metal_(API) CUDA Related to the CUDA backend labels Sep 14, 2026
feal87 added a commit to feal87/myllama.cpp that referenced this pull request Sep 14, 2026
Fuse the per-element gate into hc_pre and the identity comb into hc_post
(CPU + CUDA), used by the qwen4exp graph.

From ggml-org#28901

Assisted-by: deepseek-v4-flash
@pwilkin

pwilkin commented Sep 16, 2026

Copy link
Copy Markdown
Member

Before:

model size params backend ngl test t/s
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 pp512 334.85 ± 43.96
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 tg128 20.96 ± 0.61
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 pp512 @ d30000 232.26 ± 10.62
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 tg128 @ d30000 16.85 ± 0.92

After:

model size params backend ngl test t/s
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 pp512 360.32 ± 23.06
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 tg128 22.11 ± 0.06
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 pp512 @ d30000 228.14 ± 24.20
qwen4exp A3B Q4_K - Medium 103.68 GiB 176.94 B ROCm,Vulkan -1 tg128 @ d30000 16.81 ± 1.17

Tested mixed mmproj+text input, everything looks fine.

@am17an
am17an merged commit 37b53fd into master Sep 16, 2026
45 of 48 checks passed
quimmedes pushed a commit to quimmedes/cafe-llama.cpp that referenced this pull request Sep 16, 2026
SelfRef added a commit to SelfRef/llama.cpp-rdna that referenced this pull request Sep 18, 2026
Assisted-by: Claude Opus 5
Te-eMster pushed a commit to Te-eMster/mx-llama.cpp that referenced this pull request Sep 19, 2026
x1250 added a commit to x1250/llama.cpp that referenced this pull request Sep 22, 2026
The ggml side of upstream 37b53fd: ggml_dsv4_hc_pre_gated (a per-element
gate variant of dsv4_hc_pre, scale in op_params[0], gated flag in
op_params[1]) and ggml_dsv4_hc_post with a null comb (identity), with the
CPU and CUDA implementations and the Vulkan, SYCL and Metal supports_op
guards that reject the new variants. The src/models/qwen4exp.cpp half is
not taken: this fork keeps its own fused hyper-connection graph
(ggml_hc_gated_mean, ggml_hc_inject, the merged hc_*_down_inject weights),
which runs on Vulkan, where upstream's variants are rejected.

(cherry picked from commit 37b53fd, ggml/ paths only)
@BrewTestBot BrewTestBot mentioned this pull request Sep 23, 2026
1 task done
despc added a commit to despc/llama.cpp that referenced this pull request Sep 26, 2026
Conflicts resolved by hand, keeping the fork's functionality:
- allreduce.cu: our mixed-runtime collective kept; upstream's HIP enablement
  and CUDA_CHECK cleanup applied, #else stub comment taken from upstream.
- llama-context.cpp/.h: upstream's per-graph-type gf_res_prev and
  gf_res_prev_active (ggml-org#28549, CUDA graph for MTP draft) around our ubatch
  phase profiler; expert heatmap/hotstore members kept.
- server-context.cpp/.h: upstream's handle_count_tokens signature (ggml-org#29309)
  with our reject_unknown_model guard.
- fattn-common.cuh: our GGML_CUDA_FATTN_LAUNCH_PROFILE log kept.
qwen4exp.cpp auto-merged: our MTP head kept, upstream hc ops and sparse FA in
(PR ggml-org#28243 still unmerged upstream, so MTP stays ours).

Validation (greedy, temp 0 / top_k 1 / seed 42, hash of reasoning+content):
- Qwen3.8-27B 4-GPU MTP: byte-identical to the old build on all prompts.
- Qwen3.8-Flash 4-GPU MTP: 1 of 3 identical, 2 diverge late. Cause is the
  fused hc_pre_gated/hc_post ops from ggml-org#28901 (different rounding order);
  still diverges with GGML_CUDA_DISABLE_FUSION=1. test-backend-ops passes
  DSV4_HC_PRE/POST on CUDA0, CUDA1, V100_CUDA0, V100_CUDA1. Perplexity
  (docs corpus, 24x1024): 5.4967 +/- 0.127 old, 5.4707 +/- 0.126 new.
Generation speed unchanged on both models.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Apple Metal https://en.wikipedia.org/wiki/Metal_(API) CUDA Related to the CUDA backend ggml changes relating to the ggml tensor library for machine learning model Model specific SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language testing Everything test related Vulkan Issues specific to the Vulkan backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants