qwen36 tier: QT_MAX_ROWS, placement pin, ownership-based int8 free (follow-up to #1344) - #1388
Conversation
…ainer docs Review response for JustVugg#1344, rebased onto dev after the auto-placer (ff13134) moved the is_x allocation below the placement pass. - QT_MAX_ROWS replaces the bare 32 in all seven sites that must agree (is_k width, K clamp, tg/tu/td, rows[], topk message, is_x_floats, qt_issue stride). JustVugg#1339 was three of these disagreeing. - multidev test gains the sizeof consistency check and a placement pin. These catch different drift directions: widening is_k to 64 while the stride stays 32 is caught ONLY by the sizeof check (placement, in-bounds and disjointness all pass); shrinking the stride below the row capacity is caught by the placement pin where in-bounds is not sufficient. Both kept. - warmstart log line and docs no longer promise an RSS saving that does not exist on an int8 container (since JustVugg#1341 the free is int4-only). The shutdown-test watchdog from the original follow-up is already on dev (fa8142d), so that file is untouched here.
The JustVugg#1341 free keyed on expert_is_int4, which encodes today's correspondence between weight format and memory ownership. A third format that also aliases e->g would have no reason to read a format flag as an ownership statement and would reintroduce the use-after-free. State the invariant directly: do not free what was just handed to the tier. int4 handed g4, so the int8 copy is spare and goes; int8 handed e->g itself, so it stays. Same behaviour on both current containers (test_qwen36_tier_int8_engine covers both), correct by construction for the next one. Suggested by JustVugg in the JustVugg#1344 review.
cnt was incremented and never read, and -Wunused-but-set-variable flagged it in every test build that includes qwen36_tier.c. The engine build must stay at zero warnings; now the test builds do too.
kreuzzelg
left a comment
kreuzzelg
left a comment
There was a problem hiding this comment.
Thanks for taking all three points from the #1344 review; this closes them.
QT_MAX_ROWS. All seven sites now read the one constant, and the sizeof-based check in the multidev test ties is_k's row capacity to the replica stride, which is exactly the pair #1339 let drift. The two placement pins (records[dev0].x == G.is_x, records[dev1].x == G.is_x + QT_MAX_ROWS*D) are the assertion the original test lacked: in-bounds and disjoint both pass on a single-row issue under the old 8*D stride, these do not. One literal is left in the test itself, float val[32] and its loop just below the new pins; harmless, but since the point of the PR is that the number lives in one place, QT_MAX_ROWS there too.
Ownership free. !keep8 && e->g && wg != (const uint8_t *)e->g is the right predicate: int4 hands g4 (spare int8 copy, free it), int8 hands e->g itself (keep it), and a third format that aliases e->g is safe without a format check. The wg == NULL case still leaves via the continue above it, so qt_note_planned has always staged before anything is freed. Correct by construction, as the description says.
Log line and docs. Both now say which container they describe; the 40 → 29 GB row is labelled as int4-only. Good.
On real hardware, two cards (RTX 3070 + Quadro RTX 4000, driver 580.173, CUDA 12.0), this PR plus #1390 applied on dev 6f9117f: the CI qwen36_tiny fixture as an int8 container (--ebits 8, ./qwen36 8 8), COLI_CUDA=1, default warmstart: 40/40 runs, 16/16 tokens, uploads 64 / miss(CPU) 0 / VRAM hit rate 100 %, banner int8 container: all experts keep their weights in RAM. Same fixture as int4 (--ebits 4, ./qwen36 8 4): 20/20, 16/16, banner int8 copy dropped for residents, kept for non-residents, resident 64/64. All seven tests/test_qwen36_tier_* pass. (Caveat as in #1390: dev's warmstart has an unrelated one-in-twelve segfault in ehit_mark's lazy allocation, see #1331; the runs above carry a local lock for it.)
Approve. My open qwen38 tier branch adds to qwen36_tier.c too (fp8 mode, dense residents) but not to qt_issue or the is_x sizing, so rebasing onto this is mechanical; I will do it once it lands.
Second forward-merge of this branch. Four files conflicted; the rest of dev merged clean. c/qwen36_tier.c, c/qwen36_tier.h: JustVugg#1390 landed upstream with reworded comments, so the branch's copy of the qt_fill_wait fix is dropped in favour of dev's. JustVugg#1388's QT_MAX_ROWS replaces the literal 32 in the issue path and the per-device row arrays; the backend-neutral QtTensor stays, since dev's side of those lines still spells ColiCudaTensor. The dense-trunk block dev added for the Qwen3.8 trunk (qt_dense_init, qt_dense_matmul, dense_free_all) called coli_cuda_* directly and would not compile without CUDA. It now goes through the same be_trunk_upload / be_trunk_matmul / be_free shim as lm_head and the DeltaNet projections, so on Vulkan it refuses and the matrices stay on the CPU path. coli_cuda_group_stats is left alone: it is already inside #ifdef COLI_CUDA. G.ybuf was the one remaining literal 32 in the file and is now sized by QT_MAX_ROWS. Not a live overflow at the current value, but it is the fourth spelling of the constant JustVugg#1339 was filed to unify. c/Makefile: dev's new prerequisites (expert_ffn.h, omp_tune.h, kv_prefix.h) with this branch's $(VK_OBJ)/$(VK_SPV) re-applied, and the tier condition is dev's ifneq over CUDA/CUDA_DLL/HIP/HIP_DLL with the VK=1 branch kept after it. The three tests/test_qwen36_tier_vk* rules are unchanged. docs/qwen36.md: dev's expert-kernel section and the ROCm paragraph, with the VK=1 sentence and the qwen36-tier.md link restored.
Follow-up to #1344, addressing the review there from @kreuzzelg and @JustVugg. Three commits on current
dev.1. Review response (original implementation by @mfethe1, crichalchemist#1)
QT_MAX_ROWSreplaces the seven literal32s inqwen36_tier.cthat had to agree (is_k, theK>clamp,tg/tu/td,rows[], the topk clamp message,is_x_floats, theqt_issuestride). qwen36 CUDA tier: qt_issue overruns G.is_x with two or more GPUs (stride 8*D into a 32*D buffer) #1339 was three of them disagreeing. Since fix(qwen36): three CUDA expert-tier bugs (#1339, #1340, #1341) #1344 theis_xallocation moved below the auto-placer; the constant follows it there.test_qwen36_tier_multidev: asserts device 0's block starts atG.is_xand device 1's atG.is_x + QT_MAX_ROWS*D, plus thesizeof-based stride check. They detect different drifts: a stride reverted to8*Dtrips the pin (in-bounds and disjointness both still pass on a single-row issue), while wideningis_kwithout touching the stride trips only thesizeofcheck. Both kept.docs/qwen36-cuda-tier.mdsay which container they describe: on an int8 container nothing is freed since qwen36 CUDA tier: int8 containers free the weights the tier still points at (regression from #1334) #1341, and the 40 → 29 GB RSS row is an int4 measurement with no int8 analogue.The shutdown-test watchdog from the original follow-up is already on
devvia fa8142d, so this PR does not touchtest_qwen36_tier_shutdown.c.2. The #1341 free keys on ownership, not on format
@JustVugg's suggestion from the review.
if (!keep8 && expert_is_int4 && e->g)encoded today's correspondence between weight format and memory ownership; a third format that also aliasese->gwould reintroduce the use-after-free without anyone reading a format flag as an ownership statement. Now:Do not free what was just handed over. Same behaviour on both current containers (
test_qwen36_tier_int8_enginecovers both), correct by construction for the next one.3. One warning
cntin the placement pricing loop (ff13134) was set and never read;-Wunused-but-set-variableflagged it in every test build that includesqwen36_tier.c. Removed.Verified
macOS 13 x86_64 (2017 iMac, i7-7700K), Apple clang, fake CUDA backend:
make qwen36and all seventests/test_qwen36_tier_*binaries with 0 warnings, all passing (autoplace, fp8, int8, int8_engine, invariants, multidev, shutdown). Not built on MinGW/UCRT64 locally; the Windows job here is the proof for that.