Repository navigation
imatrix: read activations through strides, all planes, and F16/BF16 - #31
Merged
Merged
Conversation
The dense collector read src1 as packed f32 and walked ne[1] rows only, so strided row views, batched ne[2]/ne[3] planes and F16/BF16 activations were summed from the wrong memory or skipped. Rows are now addressed through nb[1..3], every plane is accumulated into the weight's column vector, and F16/BF16 rows are converted with the ggml row helpers. Other types, or a strided nb[0], are refused with an error and counted instead of being read as floats. The device copy is sized by ggml_nbytes so a strided view no longer overruns it. The MUL_MAT_ID branch uses the same row reader. Saving the imatrix logs how many collected matmuls saw each layout.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IMatrixCollector::collect_imatrixread the activation operand of a denseMUL_MATas a contiguous F32 matrix and looped overne[1]only. It ignored the row stride (nb[1]), the batch planes (ne[2],ne[3]), and F16 / BF16 inputs, which were reinterpreted as floats. The device-copy path also sized its host buffer from the element count rather than the byte size.This change:
MUL_MAT_ID(MoE) branch;Results
Synthetic harness driving the unchanged collector method: master fails the strided, batched, permuted and F16 cases (relative error up to 4.5) and aborts on BF16; with this change all 7 cases pass (error at most 2.1e-7).
Real calibration graphs, matmul operand layouts counted during collection:
The imatrix files are identical before and after the fix on all of these, so existing calibrations for these families are unaffected. The fix matters for graphs that do hand the collector strided, batched or half-precision operands; Wan, LTX, Qwen-Image and Z-Image graphs and the Metal, Vulkan and ROCm backends were not checked.