Skip to content

[CPU] Fix accumulation precision in affine quantized matmul - #4803

Open
erwinzhang7 wants to merge 2 commits into
ml-explore:mainfrom
erwinzhang7:fix-cpu-qmm-bf16-accum
Open

erwinzhang7 wants to merge 2 commits into
ml-explore:mainfrom
erwinzhang7:fix-cpu-qmm-bf16-accum

Conversation

@erwinzhang7

Copy link
Copy Markdown
Contributor

Fix precision loss in CPU quantized matmul by accumulating in float32 instead of bfloat16 or float16.

The scalar _qmm_t path accumulates in T sum, while _qmm accumulates directly into the low-precision output buffer. This repeatedly rounds intermediate sums, causing substantial errors for large reduction dimensions.

The fix:

  • Uses a float accumulator in _qmm_t.
  • Uses a per-row float accumulation buffer in _qmm, converting to the output dtype once.
  • Adds a deterministic regression test covering 84 combinations of dtype, layout, group size, and quantization bits.

Dequantization arithmetic and existing SIMD paths are unchanged.

Validation

Tested on M5 Max (18-core), macOS 27.0.1, CPU-only MLX builds.

  • Regression test: 78/84 subtests fail on main; all pass with the fix.
  • Python suite: 59 tests pass (16 skipped).
  • C++ suite: 276/276 cases, 4433/4433 assertions pass.

For a 4096×4096 quantized matmul (4-bit, bf16):

Metric Main Fixed
Max absolute error (transposed) 0.682 0.0233
Max absolute error (non-transposed) 0.613 0.0261
Transposed latency (M=1) 72.9 ms 26.9 ms
Non-transposed latency (M=1) 14.9 ms 11.6 ms

The fix reduces maximum error by approximately 29× and improves transposed bf16 performance by 2.7×.

Remaining bf16 discrepancies come from low-precision dequantization and multiplication, which are intentionally outside this change.

  • ☑️ I understand it is strictly prohibited to use AI to write PR description
  • AI usage disclosure: Claude Code reviewed and worked with me to improve my fix

Comment thread mlx/backend/cpu/quantized.cpp Outdated
constexpr int bytes_per_pack = get_bytes_per_pack(bits);
constexpr int packs_in_group = group_size / pack_factor;

std::vector<float> acc(N);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of allocating a temporary memory with std::vector directly, can you allocate memory with MLX's allocator and register it with encoder's add_temporary and set_input_array like what GPU backends would do with intermediate buffers? It will make use of our memory cache, and will be required when we parallelize CPU scheduler.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yessir done.

@erwinzhang7
erwinzhang7 requested a review from zcbenz November 2, 2026 22:49
@zcbenz
zcbenz force-pushed the fix-cpu-qmm-bf16-accum branch from 0823c38 to b434bdb Compare November 3, 2026 03:11

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants