fix(cpu/matmul): widen GEMV weight row offsets to long (#429)

[jamesburton]

Closes #429.

Widens the two GEMV weight row-offset expressions in src/DotLLM.Cpu/Kernels/MatMul.cs to long, matching the (long)mStart * ... convention the Gemm paths already use.

• ComputeRows — weightsQ8 + row * rowBytes, 7 sites across the AVX-512+VNNI, AVX-512, AVX2 and scalar tiers.
• GemvF16 — weightsHalf + row * k, 2 sites.

Out of scope exactly as the issue specifies: activation-buffer offsets are untouched, and ComputeGemmTiled's row loop (row < tileRows <= 256, base pre-offset via (long)mStart * q8RowBytes) was already safe, so the tiled F16 row read at line 1624 is deliberately left as-is.

Confirming the reachability analysis

I reproduced both calculations. @unsafePtr's follow-up correction is right, and it matters for how this should be prioritised:

site	stride	Llama 3.1 405B LM head (vocab 128256, hidden 16384)	vs int.MaxValue
ComputeRows	17408 bytes/row	128256 × 17408 = 2,232,680,448	over by ~85 MB
GemvF16	16384 elements/row	128256 × 16384 = 2,101,346,304	~2.2% under

So site 1 is reachable on a shipping model — first affected row 123,362, meaning the last 4,894 rows (3.8% of vocab) read from a wrapped negative offset — while site 2 needs a tensor larger than anything current. Both are still one-line widenings, so both are fixed here.

Performance

The AC asks for a before/after benchmark since ComputeRows is the Q8_0 decode hot path. Timing could not answer this on my hardware, so I diffed the JIT's x64 instead, which turned out to be decisive.

The baseline was already sign-extending — just after the multiply. The 4-row loop body:

BASELINE                          THIS PR
mov      ecx, r14d      ; row     mov      eax, r12d     ; rowBytes
imul     ecx, r12d      ; 32-bit  imul     rcx, rax      ; 64-bit
movsxd   rcx, ecx       ; widen                          ; row already widened

The cast does not add widening, it moves it ahead of the multiply — which is precisely the fix. That path comes out one instruction shorter; the scalar tail gains a spill/reload of the widened row. Total method size 1024 → 1028 bytes (+4). All of it sits in the outer row loop, amortized over 16–128 VNNI block iterations of inner work per row, so there is no mechanism for a measurable regression.

For completeness, the timing attempt: both DotLLM.Cpu.dlls loaded into one process via separate AssemblyLoadContexts, interleaved per-invocation in position-balanced BASE, CHG, CHG, BASE groups, 8 runs with slot assignment swapped halfway, plus A/A controls.

shape (m×k)	base median	this PR median	within-arm run-to-run spread
4096×512	0.145 ms	0.157 ms	46%
11008×4096	5.10 ms	5.94 ms	88%
32000×4096	16.68 ms	16.69 ms	78%

Within-arm spread swamps every delta, and the deltas disagree in sign and magnitude across shapes. A/A controls came in under 1%, so the harness itself was sound — the machine simply is not quiet enough to resolve a change this small. Worth flagging one trap I hit: a ~30% bimodal artifact attached to whichever assembly loaded second, not to either DLL. Without the swapped-slot control I would have reported a false 33% regression.

Testing

--filter "FullyQualifiedName~MatMul|FullyQualifiedName~Gemv|FullyQualifiedName~Gemm" → 207 passed, 0 failed, 7 skipped.

No regression test accompanies this. Triggering it needs a >2 GB tensor allocated and mapped, which is not something a unit test can reasonably do — I did not want to add a test that only appears to cover the case. Happy to add one if you would rather have it gated behind an explicit opt-in environment variable.

[Copilot]

Pull request overview

This PR addresses issue #429 by widening GEMV weight row-offset arithmetic in the CPU matmul kernels to long, aligning GEMV’s pointer math with the existing GEMM conventions and preventing int overflow for very large (multi‑GB) weight matrices.

Changes:

• Widened Q8_0 GEMV row pointer arithmetic in ComputeRows to avoid 32-bit overflow on large tensors.
• Widened F16 GEMV row pointer arithmetic in GemvF16 to avoid 32-bit overflow for large m*k element counts.

Suppressed comments (2)

src/DotLLM.Cpu/Kernels/MatMul.cs:162

• Only the first of the four row pointers is widened to long. The (row + 1/2/3) * rowBytes expressions still execute in 32-bit int arithmetic and can overflow for the same >2GB Q8_0 tensors, leading to incorrect/out-of-bounds reads for rows near the end of the matrix.

                    weightsQ8 + (long)row * rowBytes,
                    weightsQ8 + (row + 1) * rowBytes,
                    weightsQ8 + (row + 2) * rowBytes,
                    weightsQ8 + (row + 3) * rowBytes,

src/DotLLM.Cpu/Kernels/MatMul.cs:180

• Only the first of the four row pointers is widened to long. The (row + 1/2/3) * rowBytes expressions still execute in 32-bit int arithmetic and can overflow for the same >2GB Q8_0 tensors, leading to incorrect/out-of-bounds reads for rows near the end of the matrix.

                    weightsQ8 + (long)row * rowBytes,
                    weightsQ8 + (row + 1) * rowBytes,
                    weightsQ8 + (row + 2) * rowBytes,
                    weightsQ8 + (row + 3) * rowBytes,

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jamesburton]

Thanks — this catch is correct and it mattered. Fixed in 09242339.

The review is right and my original fix was incomplete. I widened weightsQ8 + row * rowBytes but left the three sibling pointers of the 4-row unrolled calls — weightsQ8 + (row + 1|2|3) * rowBytes — in 32-bit arithmetic, across all three unrolled tiers. Those overflow on exactly the same tensors, and marginally sooner, since they address further into the matrix. So the specific hot path this PR exists to protect was still exposed: on the Llama 3.1 405B LM head the base pointer would be correct while the +1/+2/+3 rows wrapped, giving a partially-corrupt 4-row block rather than a clean failure.

Worth naming the process failure, since it is the reusable lesson: I applied the change with a regex on weightsQ8 + row * rowBytes and then verified with a grep for the same pattern. The check could not fail — it was the edit expressed twice. The correct check was a pattern matching any row-derived multiply, which is what I used this time:

grep -rnE "\+ \(?row( \+ [0-9]+)?\)? \* (rowBytes|k)" src/DotLLM.Cpu/Kernels/ | grep -v "(long)"

That now returns exactly one hit in this file — the tiled F16 row read at line 1624 — which stays as-is deliberately: row < tileRows <= 256 and the base is pre-offset via (long)mStart * k.

Also widened in the same commit: GemvF32's two weight row offsets (a + row * k). a is documented as "weight matrix A [M×K]", so these are the same class as the GemvF16 sites already covered here. Wrapping needs >2^31 float elements (~8.6 GB), so like F16 this is consistency rather than a reachable defect — flagging it explicitly since it slightly widens the diff beyond what #429 lists.

Re-tested: --filter "FullyQualifiedName~MatMul|FullyQualifiedName~Gemv|FullyQualifiedName~Gemm" → 206 passed, 0 failed, 7 skipped.

The codegen argument in the PR description is unaffected — the widening still moves ahead of the multiply on every one of these sites, and it all remains in the outer row loop.

[unsafePtr]

Why there are so many open PRs created by you? Like this it looks like created fully automatically.

[jamesburton]

Pushed 58cdf4cb — comments only, no behaviour change.

I ran a follow-up audit of the whole CPU backend for this bug class rather than just the sites #429 lists, to check nothing else was lurking. Result: no further reachable sites. The widenings in this PR cover the CPU weight-offset surface.

What that audit did change is that the two tiled F16 row reads (GemmF16 and GemmTiledF16Worker) now carry a comment recording why they stay int — they look exactly like the bug, and without a note they will keep attracting well-meant "fix" attempts. The bound: the tensor-scale offset is already carried in 64-bit by tileWeightsHalf ((long)mStart * k), and row < tileRows <= tileM <= 256 because every TileM in the project originates from ComputeTileM's Math.Clamp(tileM, 4, 256) — so the residual product tops out at 255 * k, about 4.2M for a 16384-wide 405B tensor.

Worth noting for the record that several sites would have been live bugs had they been written with int: DeepSeek-V3's Q6_K expert offsets reach 3,070,771,200, well past int.MaxValue. They are already long in MoeQuantSwiGluMlp / MoeSwiGluMlp. That is the existing code getting it right, not luck.

One thing I deliberately left alone, flagging it rather than quietly deciding: the caller-supplied preQuantizedInput GEMM paths (MatMul.cs, MatMulKQuants.cs, MatMulQ5_0.cs) are the only activation-offset sites not structurally bounded by an int-sized internal ArrayPool.Rent — the buffer comes from outside, so the self-limiting argument rests on the caller rather than on the kernel. Wrapping still needs ~123k tokens in a single un-chunked prefill, and #429 explicitly scopes activation offsets out, so I kept the diff honest to the issue. It is three free (long) casts on loop-invariant expressions if you would rather have belt-and-braces.

The same audit found real sites on the CUDA side, which are not in scope here — filed separately as #432 with #433 for the fix.