feat(gemma4): route target prefill through pflash - #179
Conversation
73b4548 to
11c5963
Compare
7694057 to
3d04a92
Compare
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
The test calls pflash_register_ggml_kernel(), which is defined in
src/pflash_ggml_adapter.cpp. That source is only compiled into
dflash27b on CUDA + sm>=80 (the elseif branch at line ~291). The test
target was unguarded, so CI runners building at sm<80 successfully
built dflash27b without the adapter, then failed to link the test:
undefined reference to `pflash_register_ggml_kernel()'
Mirror the same guard used by test_flashprefill_kernels at line ~396.
Plain-C++ compile units that include internal.h (e.g. smoke tests built with g++ rather than nvcc) hit a fatal error on the unconditional #include <cuda_runtime.h> guarded by GGML_USE_CUDA. The runtime header is only needed by src/cuda_cross_device_copy.cpp (the implementation TU), which already includes it directly. Replace the header include with a forward declaration of cudaStream_t (typedef struct CUstream_st*) so consumers of the dflash_cuda_copy_ between_devices prototype don't need CUDA include paths. Found via CI failure on the smoke_load_gemma4_target / smoke_gemma4_target_forward targets after CI started reaching them following the test_flash_attn_sparse cmake guard fix.
3d04a92 to
2d7d084
Compare
The smoke driver was leaving GemmaGraphInputs::swa_mask null, causing gemma4_target_graph SWA layers to fall back to attn_mask via the effective_mask = swa_mask ?: attn_mask path. attn_mask is sized for the full-attn view (kv_len_padded), but SWA layers view the full swa_ctx_alloc slots, so flash attention reads past the end of attn_mask into adjacent GPU memory. Manifests as all-NaN logits with Q4_0 / TQ3_0 KV (the OOB bytes are interpreted as fp16 mask values added to attention scores). Q8_0 tolerated the OOB read by accident; Q4_0 / TQ3_0 do not. The bug is documented in gemma4_runtime_helpers.cpp:124-130 — the runtime helper used by the daemon path sets swa_mask correctly. The smoke driver bypassed that helper. Allocate swa_mask sized [align_up(swa_ctx_alloc, 256), n_tokens] and fill the same causal pattern as attn_mask. Caught by running the test on a real GPU (CI only compiles, doesn't run). No production code changed.
2d7d084 to
c8cb9d8
Compare
ggml_flash_attn_sparse has no mask argument. When its TQ3 sparse path can't handle a prefill shape, it delegates to ggml_cuda_flash_attn_ext, which routes TQ3 to BEST_FATTN_KERNEL_CHUNKED only when mask != nullptr (fattn.cu:438-466). Without a mask the dispatcher returns NONE and aborts at fattn.cu:572-576. Bench-validated reproduction: --pflash -ctk tq3_0 -ctv tq3_0 on a 6-token Gemma4-31B prompt aborts in prefill. After this gate, TQ3+pFlash falls back to dense FA + CHUNKED for prefill (mask explicit) and uses sparse FA + TQ3 VEC for decode only (n_tokens == 1). Headline numbers post-fix (RTX 3090, Gemma4-31B Q4_K_M): - Greedy temp=0 first 2 tokens match Q8 baseline (reproduces PR Luce-Org#10 claim) - Long-context prefill: +9% @ 16k, +27% @ 32k, +52% @ 64k vs Q4_0 KV See dflash/docs/gemma4-pr-split/pr13-review-findings.md (Finding 5).
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dflash/src/gemma4_target_loader.cpp">
<violation number="1" location="dflash/src/gemma4_target_loader.cpp:675">
P1: Integer-overflow-prone file range check may allow OOB mmap reads on malformed tensor offsets/sizes</violation>
</file>
<file name="dflash/src/gemma4_target_graph.cpp">
<violation number="1" location="dflash/src/gemma4_target_graph.cpp:912">
P2: PLE slice math is unsafe for 2D [n_embd_per_layer, n_layer] input</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!t) continue; | ||
| const size_t off = data_start + gguf_get_tensor_offset(gctx, tid); | ||
| const size_t sz = gguf_get_tensor_size(gctx, tid); | ||
| if (off + sz > mm.len) { |
There was a problem hiding this comment.
P1: Integer-overflow-prone file range check may allow OOB mmap reads on malformed tensor offsets/sizes
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dflash/src/gemma4_target_loader.cpp, line 675:
<comment>Integer-overflow-prone file range check may allow OOB mmap reads on malformed tensor offsets/sizes</comment>
<file context>
@@ -0,0 +1,753 @@
+ if (!t) continue;
+ const size_t off = data_start + gguf_get_tensor_offset(gctx, tid);
+ const size_t sz = gguf_get_tensor_size(gctx, tid);
+ if (off + sz > mm.len) {
+ set_last_error(std::string("tensor '") + tname + "' overflows file");
+ gguf_free(gctx);
</file context>
| if (off + sz > mm.len) { | |
| if (off > mm.len || sz > mm.len - off) { |
| ggml_tensor * ple_emb; | ||
| if (ggml_n_dims(in.per_layer_inp) >= 3 || (int)in.per_layer_inp->ne[1] == w.n_layer) { | ||
| // Shape [n_embd_per_layer, n_layer] or [n_embd_per_layer, n_tokens, n_layer] | ||
| ple_emb = ggml_view_2d(ctx, in.per_layer_inp, |
There was a problem hiding this comment.
P2: PLE slice math is unsafe for 2D [n_embd_per_layer, n_layer] input
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dflash/src/gemma4_target_graph.cpp, line 912:
<comment>PLE slice math is unsafe for 2D [n_embd_per_layer, n_layer] input</comment>
<file context>
@@ -0,0 +1,1007 @@
+ ggml_tensor * ple_emb;
+ if (ggml_n_dims(in.per_layer_inp) >= 3 || (int)in.per_layer_inp->ne[1] == w.n_layer) {
+ // Shape [n_embd_per_layer, n_layer] or [n_embd_per_layer, n_tokens, n_layer]
+ ple_emb = ggml_view_2d(ctx, in.per_layer_inp,
+ n_embd_per_layer, n_tokens,
+ in.per_layer_inp->nb[1],
</file context>
The env-read lambda for DFLASH_PFLASH_TQ3 lived inside the per-layer attention-block builder, declared `static const` so initialization happens once. The static guards mean it WAS already cheap after the first call, but the placement made the env read look per-layer to readers and put a getenv call in the hot per-layer-per-graph code path (albeit one whose value never changes after the first init). Move the `static const bool s_pflash_tq3` initializer up to file scope alongside `EPS`. `pflash_supports` continues to read it as a TU-local constant. Behavior is byte-identical to pre-hoist. See dflash/docs/gemma4-pr-split/pr13-slop-audit.md (Additional findings — hot init in lambda).
… v2) F5 v1 (a292818) gated pflash sparse FA to decode-only when KV was TQ3_0 to avoid the no-mask CHUNKED-rejection abort at fattn.cu:576. That fixed AR+TQ3+pflash. F5 v2 extends to decode-only regardless of KV type, after MTP+TQ3+pflash on Gemma4-31B was found to still segfault. Root cause: when MTP is loaded, init's resolve_mtp_donor_layers tags cache.mtp_last_full_layer and create_gemma4_cache's asymmetric KV override downgrades that donor layer's K/V from TQ3_0 to Q8_0 (so the MTP cross-attention can read it). For that layer Kfa/Vfa.type is Q8_0, the v1 `tq3_kv` predicate is false, and v1 then permits pflash sparse FA on head_dim=512 + n_tokens>1. Sparse FA has no kernel for that shape; its no-mask fallback to ext fa rejects CHUNKED (mask=null) and aborts. Tightened gate: pflash only when `n_tokens == 1`. Verified with DEBUG_F5 stderr trace: layer 59 was the offender (head_dim=512, Kfa.type=q8_0 after MTP load, n_tokens=7 prefill → can_pflash=1 in v1). Post-fix: - AR + TQ3 + pflash: ok (24.3 tok/s on 7-tok prompt) - MTP + TQ3 + pflash: ok (24.3 tok/s, was segfault pre-fix) The `tq3_kv` predicate is retained as `(void)tq3_kv` because future work may grow sparse-FA kernels for non-TQ3 head_dim=512 prefill, at which point the gate can re-soften. See dflash/docs/gemma4-pr-split/pr13-review-findings.md (Finding F5, v2 amendment).
…decode (supersedes PR Luce-Org#175 skeleton) Replaces howard0su's PR Luce-Org#175 Gemma4 skeleton with a feature-complete implementation. Howard's PR landed an AR-only backend (341 LoC) sufficient to validate the daemon protocol; this PR brings the production paths. What changes ------------ gemma4/gemma4_backend.{cpp,h} 341+91 → 1182+155 - decode_autoregressive (AR) - decode_dflash (speculative decode with DDTree drafter) - decode_mtp (Multi-Token Prediction, γ=1) - snapshot_save / snapshot_restore - park / unpark for speculative draft gemma4/gemma4_graph.cpp 448 → 1218 - iSWA target forward in pure ggml - target_feat capture (hidden states for DFlash draft KV prefill) - mtp_h_prev capture (post-output-norm hidden for MTP cross-attn) - sparse-FA dispatch via ggml_flash_attn_sparse (full-attn layers, decode-only) - F5 v2 gate (n_tokens==1) — CUDA dispatcher has no sparse kernel for head_dim=512+mask+S>1 (fattn.cu:572-576 abort) - Per-layer embedding decode handling gemma4/gemma4_loader.cpp 370 → 1197 - Per-layer-embedding (PLE) table loading - MoE expert metadata (top-k routing, expert_count, shared_exp) - Asymmetric KV override (MTP donor layers forced to Q8) gemma4/gemma4_daemon.{cpp,h} 29+20 → 43+46 - Extended Gemma4BackendConfig (draft_method, mtp_gamma, sparse_fa_alpha, draft_kv_cap_override, max_ctx) gemma4/gemma4_internal.h 184 → 17 (passthrough to ../internal.h) - Internal struct definitions consolidated into shared ../internal.h where they sit alongside Qwen3/Laguna/Qwen35 struct families (GemmaTargetWeights / GemmaTargetCache / MtpLayerWeights / SwaView / Gemma4GraphInputs / Gemma4GraphOutputs / GemmaDraftLayer / GemmaDraftWeights / MtpDrafterWeights / MtpStepGraph) - Howard's Gemma4Weights / Gemma4Cache / Gemma4Snapshot struct names are NOT carried — our pre-existing GemmaTarget* family is the canonical naming across all archs in this tree New files --------- gemma4/gemma4_runtime_helpers.{cpp,h} shared graph builders + masks gemma4_dflash_graph.cpp drafter step graph (top-level — separate model) gemma4_mtp_graph.cpp MTP step graph (top-level — separate head) include/gemma4.h public C-ish API surface Tests ----- test/gemma4/smoke_load_gemma4_target.cpp loader smoke (Dense 31B Q4_K_M) test/gemma4/smoke_load_gemma4_draft.cpp drafter smoke test/gemma4/smoke_gemma4_target_forward.cpp AR forward smoke test/gemma4/smoke_gemma4_draft_forward.cpp drafter forward smoke test/gemma4/test_gemma4_kv_tq3.cpp TQ3_0 KV cache correctness at 16k+ test/gemma4/test_mtp_loader.cpp MTP assistant GGUF loader test/gemma4/test_mtp_graph_shapes.cpp MTP step graph shape invariants Validated configurations (RTX 3090, Gemma4-31B-it Q4_K_M) -------------------------------------------------------- AR + Q4 KV @ 16k: 678 pp / 22.6 tg tok/s (+80% / +11% vs upstream) AR + TQ3 KV @ 16k: 739 pp / 16.6 tg MTP γ=1 + Q4 @ 16k: 691 pp / 21.4 tg Byte-identical to upstream llama-cli AR Q4 @ temp=0 greedy Naming clarification -------------------- The `use_pflash` / `pflash_alpha` / `s_pflash_tq3` / `DFLASH_PFLASH_TQ3` identifiers in this commit refer to the ggml_flash_attn_sparse op, NOT the PFlash product (Python speculative-prefill compression module at pflash/). A follow-up commit on this branch renames them to use_sparse_fa etc. to remove the ambiguity. Closes (when merged): Luce-Org#171, Luce-Org#176, Luce-Org#179, Luce-Org#184, Luce-Org#185 — those were the original split-PR series against pre-Luce-Org#175 main; reworked into this single PR after Luce-Org#175 brought the gemma4/ skeleton into main.
|
Superseded by #193 — re-cut against current main after #175 brought the The content of this PR is included in #193. Closing here to consolidate review surface for mrciffa. |
Validation (for review)
Upstream
llama-benchbaseline (Gemma4-31B-it Q4_K_M, q4_0 KV, FA on, RTX 3090): pp16384 = 376.62 tok/s, tg300 = 20.39 tok/s.Routes target prefill through
ggml_flash_attn_sparse(pFlash). F5 v1 (a292818) + F5 v2 (57c46ca) gate pflash to decode-only on full-attention layers — without the gate, prefill aborts atfattn.cu:576(sparse FA has no mask, dispatcher returns NONE).PR #7 of the Gemma4 split sequence. Wires the sparse-FA adapter (#170) into Gemma4 target prefill. Performance/dispatch only — no draft, no MTP, no server routing.
Depends on #177 → #176 → #171 → #170 → #169 → #168. Stacks on `split/gemma4-06-kv-correctness`; diff will include ancestor commits until they merge.
Scope (2 files, +40 / -3)
Gating logic
```cpp
const bool can_pflash = use_pflash
&& pflash_supports(Kfa->type)
&& pflash_supports(Vfa->type);
```
CMake / build defs
No new CMake hunks. The `GGML_OP_FLASH_ATTN_SPARSE` op + `DFLASH27B_HAVE_CUDA_WMMA_FLASHPREFILL` compile def landed with PR #170 (sparse-FA adapter). The op exists in the submodule (PR #169).
Test driver hunks: deferred
The spec mentions `test_gemma4_dflash.cpp` for `--pflash` coverage. That test file in `feature/gemma4-support` is 2962 lines, entangled with draft / MTP / daemon scaffolding the later PRs introduce. Extracting a pflash-only subset would either pull in symbols not yet declared (PR #8, #9) or duplicate the test file. Decision: defer the test-driver integration to the PR that introduces the driver (PR #9 dflash-runtime adds the rest of it), and rely on PR #170's `test_flash_attn_sparse` for op-level coverage in this PR. Documented for reviewer.
Risk: HIGH
First runtime path through the sparse-FA op in the Gemma4 target. Default-off behavior is identical to PR05/06; risk lives entirely behind `use_pflash = true` callers, which don't exist yet in this stack (lands with PR #9).
Review checklist (from spec)
Validation