Repository navigation
ggml-hrx: publish deferred host writebacks before a host-staging upload (engine #286) - #82
Conversation
Root cause for 1bit-MONSTER/engine#286. An HRX command program whose output binding lives in host memory stages a download buffer, enqueues the device copy and then only *queues* the memcpy to host memory: download_prepared_host_staging() calls add_host_writeback() (runtime/command-program-executor.cpp), which records {host_destination, mapped_source, size}. The memcpy itself happens in GraphReplayStreamState::mark_stream_synchronized(), and that is reached only from backend_synchronize() (ggml-hrx.cpp), cache trimming and the GGML_HRX_DIAGNOSTIC_GRAPH_SYNC path. The split scheduler does not synchronize between splits: the normal path computes every split with ggml_backend_graph_compute_async() and no sync (ggml/src/ggml-backend.cpp, "if (!sched->callback_eval)"). It synchronizes a backend only when it inserts a cross-backend input copy, i.e. only inside "if (split->n_inputs > 0)". So when an HRX split writes a tensor that lives in a host buffer and a later HRX split reads it back, upload_prepared_host_staging() uploads the previous contents of that memory. That is the gpt-oss-20b failure in engine ggml-org#286: with the expert MUL_MAT_IDs on the CPU, the gate ADD_ID output lands in a host buffer, SWIGLU_OAI (a later HRX split) reads it across the CPU split, and the host memcpy has not been applied yet. It also explains the observations the issue lists: * llama-eval-callback hides it, because that path takes the "else" branch and calls ggml_backend_synchronize(split_backend) after every node, which does flush the writeback. * GGML_HRX_DEBUG_SERIAL_EXECUTION=1 does not help, because DebugSerialExecutionTrace::sync() calls hrx_stream_synchronize() only; it never calls mark_stream_synchronized(), so the pending memcpy stays pending. * SWIGLU_OAI on the CPU is correct: the tensor is then consumed by the CPU backend, the scheduler copies it and synchronizes the producing backend. * The default MUL_MAT_ID-on-HRX layout is correct because the tensor stays device resident and no host staging is involved. Flush the pending writebacks (stream synchronize, then publish) before an upload reads host memory. The check is cheap: pending writebacks are normally empty and are cleared by the same flush. Verified by a full semantic compile of the translation unit (g++ -fsyntax-only, C++17, ggml + libhrx headers) with the same command run against a deliberately broken copy to confirm the check is meaningful. Not verified on hardware: no Strix Halo box was available, so the gpt-oss-20b KLD/greedy repro from engine ggml-org#286 has not been re-run.
09d922a to
e0f637b
Compare
|
Review (orchestrator, PR-review duty while coder-llm is down) Read against f5b7f4a. The diagnosis holds:
So an HRX split that uploads host memory written by an earlier HRX split's download reads stale bytes. That covers both upload paths: Other readers of host memory are already covered:
So the narrow fix is enough for ggml-org#286. Verdict: approve, pending the hardware run in the description (gpt-oss-20b KLD vs CPU with |
|
Verified on strixhalo (gfx1151, balanced power mode 85 W). Builds: base f5b7f4a and f5b7f4a + e0f637b, same tree and flags. The base already has the placement guard (ba0be9a), which hides the failing layout. To reach the bug, I used a test-only build of both binaries in which an env var skips the gpt-oss-20b MXFP4,
Greedy
Each config gave 3/3 identical top-5 logprobs, with no NaN. Default path, llama-bench gpt-oss-20b, 3 interleaved rounds, medians:
Both are inside the run-to-run spread: tg samples 28.27 to 28.63 on both binaries. |
a679f78
into
1bit/hrx-vulkan-patched
Root cause for 1bit-MONSTER/engine#286. PR #73 was the placement guard, which
hides the failing layout; this is the actual bug it was working around.
Root cause
An HRX command program whose output binding lives in host memory stages a
download buffer, enqueues the device copy, and then only queues the memcpy to
host memory:
download_prepared_host_staging()→add_host_writeback()records{host_destination, mapped_source, size}(runtime/command-program-executor.cpp);GraphReplayStreamState::mark_stream_synchronized();backend_synchronize()(ggml-hrx.cpp:509), cachetrimming (
runtime/graph-program-cache-limit.cpp:56) and theGGML_HRX_DIAGNOSTIC_GRAPH_SYNCpath (command-program-executor.cpp:1909).The split scheduler does not synchronize between splits.
ggml-backend.cppcomputes every split with no sync:
and synchronizes a backend only when it inserts a cross-backend input copy,
i.e. only inside
for (input_id = 0; input_id < split->n_inputs; input_id++).So when an HRX split writes a tensor that lives in a host buffer and a later HRX
split reads it back,
upload_prepared_host_staging()uploads the previouscontents of that memory.
Why this is engine ggml-org#286
With the expert MUL_MAT_IDs on the CPU, the gate ADD_ID output lands in a host
buffer, SWIGLU_OAI (a later HRX split) reads it across the CPU split, and the
host memcpy has not been applied yet. That is the missing-bias/garbage-value
signature (KLD 1.65).
It also explains every observation the issue already lists:
llama-eval-callbackhides itelsebranch and callsggml_backend_synchronize(split_backend)after every node, which flushes the writebackGGML_HRX_DEBUG_SERIAL_EXECUTION=1still wrongDebugSerialExecutionTrace::sync()callshrx_stream_synchronize()only; it never callsmark_stream_synchronized(), so the memcpy stays pending — syncing the stream is not enoughChange
Flush pending writebacks (stream synchronize, then publish) before an upload
reads host memory. The check is cheap: pending writebacks are normally empty, and
the flush clears them, so at most one extra synchronize happens per program that
reads host memory produced by an earlier replay.
This is deliberately the narrow fix — it makes the HRX backend stop assuming its
own earlier host writes are already published. The reason a tensor written by an
HRX node ends up in a host buffer at all (
ggml-allocin-place reuse ignoringbuffer_id, which let the gate ADD_ID output share the CPU MUL_MAT_ID buffer) isa separate question and is not touched here.
Verification
headers:
g++ -fsyntax-only -std=c++17 -I ggml/src -I ggml/include -I ggml/src/ggml-hrx -I <hrx>/libhrx/include ggml/src/ggml-hrx/runtime/command-program-executor.cpp→ clean. The same command against a deliberately broken copy fails, so the check is meaningful.gpt-oss-20b KLD and greedy repro has not been re-run. That is the one thing
this needs before merge:
Expected: KLD ~0.03 and correct greedy text with the placement guard still in
place, and
GGML_HRX_DEBUG_SERIAL_EXECUTION=1no longer changing the result.