Repository navigation
hrx: densify a row-padded activation once instead of falling back to the CPU (engine #991) - #210
Conversation
…es (engine ggml-org#991) The first version of the padded-tile tests used n=8, and that was not enough: BlackMamba's prefill ubatch runs these contractions at 512 tokens. The predicate itself is fine there (both cases are OK on the device against the CPU reference), so the residue that GGML_HRX_STRICT=1 reports at 512 tokens is a different gate -- the model's dt_proj activation is a non-contiguous view at that ubatch size, which the matcher refuses before any size rule. Keeping the coverage anyway: it pins the token-count half of the contract, and it is what turned an assumption into a measurement.
…the CPU (engine ggml-org#991) The SSM ports hand their small projections a column slice of a wider tensor: BlackMamba's `dt_proj` contracts 92 of the 264 columns of `x_proj`, so its activation is a view whose rows are dense but gapped. The plain dense matcher refused it on `!input->contiguous`, which put all 18 `dt_proj` nodes of the prefill ubatch on the CPU (GGML_HRX_STRICT=1 aborts on `node_31`, 18 of 1179 nodes off, 37 graph splits). The kernels bind a packed 2-D tile, so the view cannot be bound directly -- but it can be copied once. The copy path already exists for `GGML_OP_CPY` (`ggml_copy_f32` / `ggml_copy_strided_source_f32`, which takes the source extents and strides as config), so this exports that as `append_f32_densify_dispatch` and uses it from the dense matmul builder: - `common_match_mul_mat_any_format` takes `allow_row_padded_activation`; the input may then be a row-padded F32 view (`nb[0] == sizeof(float)`), and the match records `strided_activation`. - `build_mul_mat_dispatch` allocates a packed transient for exactly that node, emits the densify dispatch, and binds the transient as the activation. One copy of K*tokens floats per node per ubatch (188 KiB here), not a per-layer fallback. - The plain and unary dense matchers opt in; everything else keeps requiring a contiguous activation. Test cases build the model's shape directly (`test_mul_mat_row_padded_activation`: dense weight, activation view of a 264-wide backing at K=92 and K=232, at 512 and 8 tokens).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY
Device evidence from my own HRX build at this head (GGML_HRX=ON, amdclang, gpu-hold -l "pi prs 210+211", token granted after 3870 s at die 67 C, held 46 s).
The three new cases pass against the CPU reference, exactly as the body says:
MUL_MAT(type_a=bf16,type_b=f32,m=2944,n=512,k=92,row_stride=264): OK
MUL_MAT(type_a=bf16,type_b=f32,m=2944,n=8,k=92,row_stride=264): OK
MUL_MAT(type_a=bf16,type_b=f32,m=3712,n=512,k=232,row_stride=264): OK
Full test-backend-ops test -b HRX0 -o MUL_MAT: rc 0, 540 OK, 0 FAIL, 0 Loom compilation failed, and the % 4 boundary is preserved — m=512,n=8,k=90 still reports not supported [HRX0].
The placement claim holds, and the residual is my build's configuration, not this PR. GGML_HRX_STRICT=1 on BlackMamba-2.8B (prefill 512) at this head:
placement: 1179 nodes, 90 off HRX (MUL_MAT_ID x 54, SSM_SCAN x 18, GLU x 18), 73 graph splits
MUL_MAT is not in that list at all — the 18 dt_proj nodes the PR is about are on HRX, which is the change working. The 90 that remain are ops whose dispatchers live in the gpu-kernels add-on: my configure has GGML_HRX_HIP_ADDON_DIR:PATH= (empty), while the binary the body measured has GGML_HRX_HIP_ADDON_DIR:PATH=/home/bcloud/work/zarch/gk/addons/hip-hrx, and that tree contains exactly hip_ssm_scan_f32.hip, moe_decode_mmid.hip and glm_moe_decode.hip. So the body's "1179 nodes, 0 off HRX, 1 graph split" is the same measurement with the add-on, and my 90-off is what the same tree looks like without it. I could not reproduce "0 off / 1 split" without a rebuild against that add-on, and I would rather say that than imply it.
One instrument error of mine, since it is the fourth of the same kind today: I counted not supported with ': not supported$', which matches nothing because the line ends not supported [HRX0] , so my summary printed 0 while the k=90 line is right there. The line is what counts, not my count.
hrx: densify a row-padded activation once instead of falling back to the CPU (engine ggml-org#991)
Stacked on #208 (
8ac62a3f7); rebased onto62c0282fb. TheK % 4predicate is what lets aK=92contraction beconsidered at all, so this lands after it; it also carries the
n=512test-case commit that missed #208's merge (themerge took
9972afa1b, then=512rows were pushed after it).The gap
The SSM ports hand their small projections a column slice of a wider tensor. BlackMamba's
dt_projcontracts 92 of the264 columns of
x_proj, so its activation is a view whose rows are dense but gapped (nb[1] != 4 * 92). The plain densematcher refused it on
!input->contiguous, which put all 18dt_projnodes of the prefill ubatch on the CPU:The same contraction with a dense activation already placed (
test_mul_mat(bf16, f32, m=2944, n=512, k=92)isOK), and thenode's own shape probe reads
in(t=0 f32 ne=[92,512,1,1] c=0)— a row-padded view, not a size problem.What changes
The kernels bind a packed 2-D tile, so such a view cannot be bound directly — but it can be copied once. That copy path already
exists for
GGML_OP_CPY(ggml_copy_f32, andggml_copy_strided_source_f32, which takes the source extents and per-dimensionstrides as compile-time config), so:
append_f32_densify_dispatch(new, exported from the copy dispatch unit) fills aDispatchthat copies a strided F32 sourceinto a packed destination, reusing exactly the strided-source/kernel selection
make_copy_f32_dispatchalready does.common_match_mul_mat_any_formattakesallow_row_padded_activation: the input may then be a row-padded F32 view(
nb[0] == sizeof(float)), and the match recordsstrided_activation.build_mul_mat_dispatchallocates a packed transient for that node, emits the densify dispatch, and binds the transient asthe activation. Cost is one copy of
K * tokensfloats per node per ubatch (188 KiB here) instead of a per-layer CPUfallback; q8 packing is ruled out in that case rather than combined with it.
keeps requiring a contiguous activation.
Evidence
One
gpu-holdticket (pi-zarch-994-densify-take2: 1334 s queued behind other lanes, 169 s on the token), every item anassertion that had to hold — a printed line next to a dead run does not count (
~/work/zarch/proof/994-densify.sh):densifysymbol inlibggml-hrx.so,append_f32_densify_dispatchexported. (Take 0 failedprecisely here — see the reviewer notes.)
test-backend-ops(-b HRX0 -o MUL_MAT) — the three new row-padded cases areOKagainst the CPU reference:bf16 m=2944 k=92 row_stride=264atn=512andn=8, andm=3712 k=232 row_stride=264atn=512.Earlier classes do not regress:
m=2944 k=92andm=3712 k=232dense atn=8/n=512stillOK;k=90stillnot supported(the% 4boundary from hrx: admit element-wise dense MUL_MAT contractions at input_size % 4 (engine #991) #208). ZeroLoom compilation failedin the arm.GGML_HRX_STRICT=1exits 0 — the check that cannot be misread, since it aborts on any off-HRX node.Placement on both the prefill and the decode graph:
1179 nodes, 0 off HRX (none), 1 graph split(was 18 offMUL_MAT,37 splits). It is fully on HRX end to end.
-c 512 --chunks 4) 12.5406 — identical to the pre-change value, i.e. the copytransient is a placement mechanism, not a different computation.
13 of 5048 off (FLASH_ATTN_EXT x 13), 27 splits,GGML_HRX_STRICT=1still aborts onfattn-2(ne=[464,16,512,1],value_head_size = 464against the kernels'multiple_of(64), no partial-tile path in theprefill op). PPL 20.0704, same as before.
pp512 3653.48 ± 39.35,tg64 109.34 ± 2.65for BlackMamba;194.68 ± 1.52/7.19 ± 0.05for v1) were taken while other lanes were saturating the box and are quoted only as"the run happened", not as a result. The claim this PR makes is placement, and placement is asserted.
Residual, stated rather than smoothed over: 1 graph split remains with 0 nodes off HRX, and I have not explained it.
What this does not do
FLASH_ATTN_EXTresidue is untouched. That is the next unit, not this one.nb[0] != sizeof(float), e.g. a transpose) is still refused: the matmul kernels read a packedrow, and the densify here is a row-gap copy, not a general transpose.
Notes for the reviewer
This is the fourth round on the same class, and take 0 of this PR is the round worth reading:
namespace { … }. Internal linkage, dead in that translationunit, so clang dropped it; the extern declaration in
dispatch-copy.hstayed unsatisfied and a shared library linkshappily with an undefined reference until the first call.
ninjaexited 0, the tree was clean, and the library wasbroken — the op suite died with
symbol lookup error, and both model arms exited 127. The harness refused to read those asresults (no placement line, no PPL → arm fails), which is why the ticket came back red instead of "0 off HRX" with no
numbers behind it. Fixed by moving the definition below the anonymous-namespace close (
6dc15d36c), and the arm now assertsthe symbol with
nm -Dbefore it runs anything.k=92,row_stride=264adjacently. The case name carriesbs=[1,1],nr=[1,1],per=[0,1,2,3],k_v=0,o=0between them, so that assertion could never match and would have failed apassing run. All six patterns were re-validated offline against lines in the real output format; the assertion was the
wrong part, not the device.
not evidence. What is evidence is a
GGML_HRX_STRICT=1exit code, atest-backend-opsverdict against the CPU reference,and a symbol that is actually in the library.