Q6Kx8 packed kernel - #47
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 484788ee60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| packed: &[BlockQ6Kx8], | ||
| dst: &mut [f32], | ||
| ) { | ||
| use super::neon::gemm_q6kx8_q8k; |
There was a problem hiding this comment.
On non-NEON targets, quantized/mod.rs does not compile the neon module, but repack is compiled unconditionally and this function still resolves super::neon::gemm_q6kx8_q8k. Building candle-core on the existing x86_64/default targets will fail before the non-NEON matmul_t fallback can return its error; the driver or this import needs the same cfg gating as the NEON implementation.
Useful? React with 👍 / 👎.
| pub struct BlockQ6Kx8 { | ||
| pub(crate) d: [f16; Q4KX8_ROWS], | ||
| pub(crate) scales: [i8; Q4KX8_ROWS * (QK_K / 16)], // 128 | ||
| pub(crate) ql: [u8; Q4KX8_ROWS * (QK_K / 2)], // 1024: [chunk(2)][row(8)][64] | ||
| pub(crate) qh: [u8; Q4KX8_ROWS * (QK_K / 4)], // 512: [chunk(2)][row(8)][32] |
There was a problem hiding this comment.
These blocks are copied directly from GGUF bytes in from_bytes and mmap-cast in from_mmap, so their field order and padding are part of the persisted Q6Kx8 format. Without #[repr(C)] (and preferably a size assertion), Rust is free to change the struct layout across compiler versions/settings, which can make baked models decode the d, scales, ql, and qh fields incorrectly even though the file bytes are valid.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3329aa782d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Baked offline (via gguf-requant `--pack`); llama.cpp will not read it. The | ||
| /// Q6_K analogue of the upstream runtime-repacked Q4_K x8 path, for the residual | ||
| /// Q6_K tensors (attn_v / ffn_down). CPU-only. | ||
| Q6Kx8, |
There was a problem hiding this comment.
Adding this enum variant makes the feature-gated accelerator modules' exhaustive GgmlDType matches non-exhaustive; for example candle-core/src/quantized/metal.rs:482-500 still lists every prior dtype but has no Q6Kx8 arm, and the CUDA module has similar exhaustive matches. This breaks candle-core builds that enable those features even though the dtype is CPU-only, because Rust still typechecks those matches before any runtime bail can happen.
Useful? React with 👍 / 👎.
|
|
||
| #[cfg(target_feature = "dotprod")] | ||
| fn packed_laneq4_cache_get(rhs: &[BlockQ4K], n: usize, nb: usize) -> Arc<Vec<BlockQ4Kx8L>> { | ||
| let key = rhs.as_ptr() as usize; |
There was a problem hiding this comment.
On aarch64 dotprod prefill (m >= 4), this global cache is keyed only by rhs.as_ptr() and entries are never removed, so after a Q4_K tensor is dropped a later tensor whose allocation reuses the same address will receive the old packed weights; if the shape matches this silently produces outputs for the previous weights, and if it differs it can panic while slicing the cached pack. The cache needs to be tied to the owning QTensor/storage lifetime or include enough identity to prevent stale reuse.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab26e9deea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// pool. `dst.len()` must be a multiple of `chunk`; `f(chunk_index, &mut chunk)` | ||
| /// gets disjoint sub-slices, so it must not touch any state outside its own chunk. | ||
| /// Used to parallelize the prefill activation quantization (each tile independent). | ||
| pub fn par_chunks_mut<U>(dst: &mut [U], chunk: usize, f: impl Fn(usize, &mut [U]) + Sync) { |
There was a problem hiding this comment.
Because this safe public helper sends &mut [U] slices to barrier-pool worker threads via a raw pointer but does not require U: Send, downstream safe code can call it with non-Send element types (for example multiple Rc handles in different chunks) and clone/drop/mutate them concurrently on different threads, which Rust would normally reject and can data-race non-atomic internals. Please add a U: Send bound before exposing this as a safe parallel API.
Useful? React with 👍 / 👎.
| let mut dst_base = vec![0f32; m * n]; | ||
| let mut dst_pack = vec![0f32; m * n]; | ||
| matmul::<BlockQ6K>((m, k, n), &lhs, &rhs_t, &mut dst_base).unwrap(); | ||
| pq.matmul_t((m, k, n), &lhs, &mut dst_pack).unwrap(); |
There was a problem hiding this comment.
On non-NEON hosts, PackedQ6Kx8::matmul_t is the #[cfg(not(target_feature = "neon"))] implementation that returns Q6Kx8 packed matmul requires the neon target feature, so this unconditional unwrap makes cargo test -p candle-core packed_q6kx8_from_bytes_matches_baseline --lib fail on x86_64 before reaching the dequantization coverage. Please cfg-gate the matmul portion/test or only run the dequantization assertions off NEON.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e276d3f9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(n) => *n, | ||
| None => crate::bail!("Q6Kx8 tensor has no dims"), | ||
| }; | ||
| let packed = super::repack::PackedQ6Kx8::from_bytes(&raw_data[..size_in_bytes], n); |
There was a problem hiding this comment.
For Q6Kx8 the first dimension must describe complete 8-row groups, but this path accepts any dims[0] and constructs PackedQ6Kx8 without checking that invariant. A GGUF with a Q6Kx8 tensor such as shape [4, 256] panics in from_bytes because the computed byte count is not a full packed block, while shapes like [1, 2048] block-align and later run with groups == 0, producing zero outputs. Please bail here unless n % 8 == 0 and the packed block count matches (n / 8) * (k / QK_K).
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d5a7aa826
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| "nc8mr4" => PrefillTile::Nc8mr4, | ||
| "nc8mr2" => PrefillTile::Nc8mr2, | ||
| _ => PrefillTile::Nc4mr4, |
There was a problem hiding this comment.
When CANDLE_PACKED_PREFILL is unset, this arm selects Nc4mr4, but the Q6 driver only enables the MR=4 path when the value is Nc8mr4. In the normal unset-env configuration for packed Q6Kx8 prefill, this therefore falls through to the MR=2 path despite the code documenting nc8mr4 as the default, leaving the new packed kernel on the slower tile unless users know to set the env var.
Useful? React with 👍 / 👎.
| n: usize, | ||
| ) -> crate::Result<Self> { | ||
| let bs = std::mem::size_of::<BlockQ6Kx8>(); | ||
| if offset + byte_len > mmap.len() { |
There was a problem hiding this comment.
If a caller passes an offset near usize::MAX with a nonzero byte_len, offset + byte_len wraps in release builds, so this bounds check can succeed and as_slice later constructs a pointer outside the mapping. This is reachable through the new public from_mmap API with malformed tensor offsets; use checked_add before comparing against mmap.len().
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1024fdd29e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Q8K, | ||
| /// 8-row-interleaved Q6_K (`repack::BlockQ6Kx8`), baked offline via gguf-requant | ||
| /// `--pack`. CPU-only; the Q6_K analogue of the upstream Q4_K x8 path. | ||
| Q6Kx8, |
There was a problem hiding this comment.
When building with the cuda feature, adding this enum variant makes the exhaustive matches in candle-core/src/quantized/cuda.rs non-exhaustive; unlike the Metal path, CUDA was not given Q6Kx8 bail arms. For CUDA-enabled builds, matches such as QCudaStorage::dequantize still enumerate every dtype and will fail to compile until this variant is handled.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
No description provided.