ggml-cpu: enable Q2_0 VNNI kernel on AVX-VNNI-only CPUs - #76
Conversation
The fast path in ggml_vec_dot_q2_0_q8_0 was gated on __AVX512VNNI__ && __AVX512VL__, which is stricter than needed: the kernel body uses only 256-bit registers, so it runs unchanged on CPUs that have AVX-VNNI but no AVX512 (e.g. Intel Alder Lake / Raptor Lake). The only difference is the intrinsic name: _mm256_dpbusd_avx_epi32 instead of _mm256_dpbusd_epi32. Extend the gate with defined(__AVXVNNI__) and select the intrinsic via a local macro. Builds with -DGGML_AVX_VNNI=ON (or -march=native on supporting CPUs) now take the vectorized path. Measured on i7-12650H (Raptor/Alder Lake, no AVX512) with Ternary-Bonsai-27B Q2_g64: ~8x decode speedup vs the scalar fallback, approaching the memory-bandwidth limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
khosravipasha
left a comment
There was a problem hiding this comment.
somehow missed this one.
Thanks for adding, what is the main speed change, its on certain hardware that it fallsback?
We now have both Q1_0 and Q2_0 in main llama.cpp this could be a good cnadiate to upstream there too, can send PR there and we can later pick it up. Or can merge here as well, need to test it a bit
cc @bri-prism
There was a problem hiding this comment.
Pull request overview
Extends the Q2_0 x86 dot-product fast path to AVX-VNNI-only CPUs.
Changes:
- Broadens the VNNI compile-time gate.
- Selects the appropriate AVX-VNNI or AVX512-VNNI intrinsic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bri-prism
left a comment
There was a problem hiding this comment.
Thanks for this, and the kernel itself checks out. I verified the dpbusd operand order and saturation bounds, the 2-bit extraction across all byte values, and that the SIMD path is bit-identical to the scalar fallback. Two things need fixing before this can merge, plus some suggestions inline.
- The widened gate loses the AVX2 guarantee the old AVX512 gate carried implicitly. On MSVC we define
__AVXVNNI__outside the /arch chain (ggml-cpu/CMakeLists.txt:297), so an AVX-VNNI-only config either fails to link (hsum_i32_8lives inside the__AVX__/__AVX2__/__AVX512F__region) or, with-DGGML_AVX=ON -DGGML_AVX2=OFF, builds cleanly while emitting AVX2 encodings into an /arch:AVX binary. GCC and Clang are immune since -mavxvnni implies AVX2. Suggest:
#if ((defined(__AVX512VNNI__) && defined(__AVX512VL__)) || defined(__AVXVNNI__)) && (defined(__AVX2__) || defined(__AVX512F__))
- The commit message carries a
Co-Authored-By: Claudetrailer and the PR body a "Generated with Claude Code" footer. AGENTS.md only sanctionsAssisted-by:and lists both of these forms under prohibited usage. Please amend the commit and edit the body. Worth getting right here since this was flagged as an upstreaming candidate and ggml-org enforces this harder than we do.
One correction to the PR body: -DGGML_AVX_VNNI=ON only takes effect with -DGGML_NATIVE=OFF (the flag block sits in the else of the GGML_NATIVE branch), so the enable instructions as written are a silent no-op on most builds. Also the i7-12650H is Alder Lake (not Raptor), the artifact name says g64 where our Q2_0 is g128, and 0.54 tok/s x 8 threads over a 7.6 GB model works out to roughly 33 GB/s, which is about 43% of that part's DDR5 ceiling, so I would drop the "approaching the memory-bandwidth limit" line. The gap is good news: there is still kernel-side headroom here.
Two smaller things on lines GitHub will not let me comment on directly:
- The byte replication (movq load, two 128-bit vpshufb, vinserti128) can be one vpbroadcastq plus a single 256-bit vpshufb with a combined [idxlo|idxhi] index vector. Both lanes hold the same 8 source bytes and vpshufb is in-lane, so the result is identical. Saves 2 uops per sub-block, more on E-cores.
- Nit: the
_mm256_set_m128icall should beMM256_SET_M128I(hi, lo); the file defines the macro for a reason and this is the only raw call among 42 sites.
Not blocking, for follow-up: the per-row dpbusd(ones, qy) recompute is about 20% of the inner-loop work and would vanish if Q2_0 moved to Q8_1 activations (block_q8_1.s), but that is a cross-arch traits change for another PR. And no CI currently executes an AVX-VNNI-only config (the SDE step is commented out), so a SIMD-vs-generic bit-exactness test would be cheap insurance.
| #if (defined(__AVX512VNNI__) && defined(__AVX512VL__)) || defined(__AVXVNNI__) | ||
| // VNNI: unpack 2-bit codes c in {0,1,2,3} (value = c-1), then | ||
| // dot((c-1), qy) = dpbusd(c, qy) - dpbusd(1, qy). | ||
| // The kernel only uses 256-bit registers, so it runs unchanged on |
There was a problem hiding this comment.
This comment is not quite right: the block also uses XMM ops (the idx vectors and the movq load below), and VNNI plus 256-bit registers is not a sufficient condition, which is exactly how the gate above ended up too wide. Suggest: "uses only SSE/AVX2 ops (no 512-bit or AVX512-only instructions); requires AVX2". I would also drop the CPU model list, it is already stale (Meteor Lake, Sierra Forest).
| // The kernel only uses 256-bit registers, so it runs unchanged on | ||
| // AVX-VNNI-only CPUs (e.g. Intel Alder/Raptor Lake, where AVX512 is | ||
| // unavailable); the AVX-VNNI intrinsic differs only in name. | ||
| #if defined(__AVX512VNNI__) && defined(__AVX512VL__) |
There was a problem hiding this comment.
Consider dropping the macro and calling mul_sum_us8_pairs_float (defined ~470 lines up in this file) with the whole block gated on __AVX2__ instead. It is bit-exact for this input range: codes are masked to 0..3, so its maddubs fallback cannot saturate, and the sums stay well inside exact-float territory. That removes the fifth copy of this exact dpbusd dispatch in the tree and extends the speedup to every AVX2 CPU without VNNI (Haswell through Rocket Lake, Zen 1 to 3), including the shipped haswell/skylakex variants, which this PR currently leaves on the scalar loop.
| // unavailable); the AVX-VNNI intrinsic differs only in name. | ||
| #if defined(__AVX512VNNI__) && defined(__AVX512VL__) | ||
| #define GGML_Q2_0_DPBUSD(acc, a, b) _mm256_dpbusd_epi32(acc, a, b) | ||
| #else |
There was a problem hiding this comment.
If the macro stays, make this #elif defined(__AVXVNNI__) with a trailing #else + #error, matching the other dpbusd dispatches in quants.c, repack.cpp, and sgemm.cpp. The bare #else silently emits the AVX-VNNI intrinsic for any future config that reaches it without the feature, and __AVX512VNNI__ without __AVX512VL__ is already constructible today with -DGGML_AVX512_VNNI=ON without -DGGML_AVX512=ON.
| __m256i codes = _mm256_permute4x64_epi64(_mm256_packus_epi16(r0, r1), 0xD8); // 32 codes in order | ||
| const int dp = hsum_i32_8(_mm256_dpbusd_epi32(_mm256_setzero_si256(), codes, qy)); | ||
| const int sy = hsum_i32_8(_mm256_dpbusd_epi32(_mm256_setzero_si256(), ones, qy)); | ||
| const int dp = hsum_i32_8(GGML_Q2_0_DPBUSD(_mm256_setzero_si256(), codes, qy)); |
There was a problem hiding this comment.
The two reductions can be one: keep the dpbusd results as vectors and do hsum_i32_8(_mm256_sub_epi32(dpv, syv)). Bit-identical (lanes are bounded well inside int32) and it halves the reduction work, which is latency-serialized through vmovd. The q1_0 sibling below goes further with a float accumulator and a single hsum per call if you want to match that idiom.
| } | ||
| #undef GGML_Q2_0_DPBUSD | ||
| #else | ||
| for (int i = 0; i < nb; i++) { |
There was a problem hiding this comment.
Pre-existing, but this PR rewrites the exact #if/#else around it: this scalar branch is byte-for-byte identical to ggml_vec_dot_q2_0_q8_0_generic, and the ARM version of this function already delegates instead (arch/arm/quants.c). ggml_vec_dot_q2_0_q8_0_generic(n, s, bs, vx, bx, vy, by, nrc); return; deletes the duplicate and leaves one copy to maintain.
Problem
The Q2_0 ternary fast path in
ggml_vec_dot_q2_0_q8_0(ggml/src/ggml-cpu/arch/x86/quants.c) is gated on#if defined(__AVX512VNNI__) && defined(__AVX512VL__). That gate is stricter than the code requires: CPUs with AVX-VNNI but no AVX512 — notably Intel Alder Lake and Raptor Lake client parts, where AVX512 is fused off — fall through to the scalar loop and leave most of the available throughput on the table.Change
Extend the gate to
(defined(__AVX512VNNI__) && defined(__AVX512VL__)) || defined(__AVXVNNI__)and select the dot-product intrinsic via a local macro:_mm256_dpbusd_epi32(unchanged)_mm256_dpbusd_avx_epi32Why this is safe
__m256i) and 128-bit registers — no 512-bit state, no masking, nothing AVX512-specific.vpdpbusdoperation on ymm registers; the intrinsic name (and VEX vs EVEX encoding) is the only difference. Numeric behavior is bit-identical.Measured results
On an i7-12650H (Alder Lake, AVX-VNNI, no AVX512) running Ternary-Bonsai-27B Q2_g64 (7.6 GB): stock scalar path decodes at 0.54 tok/s; with this patch the vectorized path is enabled, giving ~8x decode speedup, approaching the memory-bandwidth limit.
Enabling it
The build system already supports this:
-DGGML_AVX_VNNI=ON(which defines__AVXVNNI__, seeggml/src/ggml-cpu/CMakeLists.txt) or the default-march=nativeon a supporting CPU enables the path automatically.Compile checks
Both configurations build cleanly (gcc 15.2,
-DGGML_NATIVE=OFF -DGGML_AVX=ON -DGGML_AVX2=ON -DGGML_FMA=ON -DGGML_F16C=ON, with and without-DGGML_AVX_VNNI=ON). Disassembly confirmsvpdpbusdis emitted inggml_vec_dot_q2_0_q8_0for the VNNI build and absent (scalar fallback) in the default build.🤖 Generated with Claude Code