Skip to content

Commit 2b927cf

Browse files
iancrismakaveli10
authored andcommitted
chore: QVAC-21914 address PR review — flush-cadence wording, nits
Non-behavioral cleanups from the PR tetherto#181 review pass: - ggml-opencl graph_compute: correct the flush-cadence comment. The old "per-token decode hot path submission-free by construction" claim was false for multi-GB models — a decode step streams the whole model, so its estimated work crosses the default 512 MB budget a few times per token. Reworded to state that accurately (cost is negligible in practice since clFlush is non-blocking, and it is tunable/zeroable to make decode fully submission-free). Mechanism unchanged. - ggml_cl_flash_attn: GGML_ASSERT(q->ne[1] <= INT32_MAX) before the int n_q truncation, since the q-chunk loop accumulates into an int and derives cl_ulong offsets from it (defensive; not reachable with real shapes). - Consistency: normalize the ticket tag to bare `QVAC-21914` (drop the `qvac ` prefix) in clip.h and tests/CMakeLists.txt, matching the .cpp files and the fork's QVAC-21257 precedent. - tests/CMakeLists.txt: move the unconditional test-opencl-fa-chunking registration up beside test-copy-tbq-subgroups (its self-skipping sibling) instead of sitting right after the LLAMA_MTMD endif() where it read as MTMD-gated; add a comment noting it deliberately does not link mtmd. No functional change to the fix; local mtmd + both tests build, test-clip-fa-cutoff passes.
1 parent 7056f4c commit 2b927cf

3 files changed

Lines changed: 26 additions & 15 deletions

File tree

ggml/src/ggml-opencl/ggml-opencl.cpp

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5798,10 +5798,14 @@ static ggml_status ggml_backend_opencl_graph_compute(ggml_backend_t backend, ggm
57985798
// from cl_a8x_cmdbuf_mgr_submit_ibs (os_exit) — observed on Galaxy S25
57995799
// Ultra / Adreno 830. Flushing whenever the estimated enqueued work
58005800
// exceeds flush_work_budget hands the driver bounded batches instead.
5801-
// Gating on WORK (not a node count) keeps the per-token LLM decode hot
5802-
// path submission-free by construction: a decode step never accumulates
5803-
// anywhere near the budget, while the giant encode flushes dozens of
5804-
// times. clFlush only submits (no host stall).
5801+
// Gating on WORK (not a node count) scales the flush cadence to the batch
5802+
// size: the ~48 s encode flushes many times, while a per-token LLM decode
5803+
// (its work ~= the model size streamed once, i.e. a few flushes per token
5804+
// at the default budget for a multi-GB model) is far lighter. clFlush only
5805+
// submits (no host stall), so the decode-path cost is negligible in
5806+
// practice (measured GPU decode TPS within noise of pre-hardening), but it
5807+
// is NOT literally zero for large models — raise flush_work_budget past the
5808+
// model size, or set it to 0, to make decode fully submission-free.
58055809
int64_t work_since_flush = 0;
58065810

58075811
for (int i = 0; i < cgraph->n_nodes; i++) {
@@ -12945,6 +12949,11 @@ static void ggml_cl_flash_attn(ggml_backend_t backend, const ggml_tensor * q, co
1294512949

1294612950
ggml_backend_opencl_context *backend_ctx = (ggml_backend_opencl_context *)backend->context;
1294712951

12952+
// QVAC-21914: n_q is int; the q-chunk loop below accumulates into an int
12953+
// (q_base += chunk_rows) and derives cl_ulong byte offsets from it. Guard
12954+
// the int64->int truncation so a pathological q-row count can't wrap
12955+
// negative and produce an out-of-bounds device offset.
12956+
GGML_ASSERT(q->ne[1] <= INT32_MAX);
1294812957
const int n_q = q->ne[1];
1294912958
const int n_kv = k->ne[1];
1295012959
const int d_head_q = q->ne[0];

tests/CMakeLists.txt

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,17 @@ llama_build_and_test(test-backend-ops.cpp)
254254
# GPU backend is available.
255255
llama_build_and_test(test-copy-tbq-subgroups.cpp)
256256

257-
llama_build_and_test(test-model-load-cancel.cpp LABEL "model")
258-
llama_build_and_test(test-model-load-disk.cpp LABEL "model")
259-
llama_build_and_test(test-model-load-memory.cpp LABEL "model")
257+
# QVAC-21914: ggml-opencl flash-attention q-chunking parity vs the CPU
258+
# backend. Unconditional and self-skipping at runtime when no OpenCL device is
259+
# present (like test-copy-tbq-subgroups above), so it exercises chunking
260+
# wherever OpenCL exists (Adreno devices, desktop OpenCL, PoCL). Does not link
261+
# mtmd, so it is intentionally outside the LLAMA_MTMD block below.
262+
llama_build_and_test(test-opencl-fa-chunking.cpp)
263+
264+
llama_build_and_test(test-model-load-cancel.cpp LABEL "model")
265+
llama_build_and_test(test-model-load-disk.cpp LABEL "model")
266+
llama_build_and_test(test-model-load-memory.cpp LABEL "model")
267+
target_include_directories(test-model-load-memory PRIVATE ${PROJECT_SOURCE_DIR}/common_test)
260268
llama_build_and_test(test-model-load-memory-split.cpp LABEL "model")
261269
llama_build_and_test(test-autorelease.cpp LABEL "model")
262270
llama_build_and_test(test-backend-sampler.cpp LABEL "model")
@@ -294,20 +302,14 @@ if (LLAMA_MTMD)
294302
target_link_libraries(${LLAMA_TEST_NAME} PRIVATE mtmd)
295303
unset(LLAMA_TEST_NAME)
296304

297-
# qvac QVAC-21914: clip flash-attention AUTO budget arithmetic (pure CPU).
305+
# QVAC-21914: clip flash-attention AUTO budget arithmetic (pure CPU).
298306
set(LLAMA_TEST_NAME test-clip-fa-cutoff)
299307
llama_build_and_test(test-clip-fa-cutoff.cpp)
300308
target_link_libraries(${LLAMA_TEST_NAME} PRIVATE mtmd)
301309
target_include_directories(${LLAMA_TEST_NAME} PRIVATE ${PROJECT_SOURCE_DIR}/tools/mtmd)
302310
unset(LLAMA_TEST_NAME)
303311
endif()
304312

305-
# qvac QVAC-21914: ggml-opencl flash-attention q-chunking parity vs the CPU
306-
# backend. Self-skipping at runtime when no OpenCL device is present, so it is
307-
# registered unconditionally and exercises chunking wherever OpenCL exists
308-
# (Adreno devices, desktop OpenCL, PoCL).
309-
llama_build_and_test(test-opencl-fa-chunking.cpp)
310-
311313
# GGUF model data fetcher library for tests that need real model metadata
312314
# Only compile when cpp-httplib has SSL support (CPPHTTPLIB_OPENSSL_SUPPORT)
313315
if (TARGET cpp-httplib)

tools/mtmd/clip.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ int clip_model_n_temporal_merge(const struct clip_ctx * ctx); // TODO @ngxson :
122122
// (and ultimately by common/fit.cpp's heuristic). Restored from upstream b9341.
123123
std::map<ggml_backend_dev_t, size_t> clip_get_mem_usage(const struct clip_ctx * ctx);
124124

125-
// qvac QVAC-21914: pure arithmetic of the flash-attention AUTO budget decision.
125+
// QVAC-21914: pure arithmetic of the flash-attention AUTO budget decision.
126126
// Returns the effective explicit-attention cutoff in n_patches given the
127127
// configured cutoff and the device memory probe:
128128
//

0 commit comments

Comments
 (0)