Repository navigation
docs(mmq): the Metal line numbers were master's, not the pin's - #453
Merged
Merged
Conversation
Third correction from the Metal host on #452, and the one worth having: the mul_mv.metal loops are at 1569 and 1607 at b11081 (161755f2), not 1572 and 1610. Those were dd266785c's -- the tree this work had open -- while the file claimed to have read the pin. mul_mm.metal and mul_mv.metal differ between the two. Verified at b11081 before taking it: 1572 is sc16[2] = ... inside the read loop and 1610 is dst_f32[first_row + row] = sum_all; inside the write loop, exactly as they said. The two quoted mul_mv.metal lines, citation comments stripped, diff clean against 1569 and 1607, whitespace included. The quoted kernel_mul_mm_id block is mul_mm.metal:542-560 verbatim, with the il0/il lines (556-558) elided where marked and the clamp comment at 552 -- checked this time by diffing the block against the pin rather than grepping its lines, which is the check the previous correction taught me to run. Their other references hold too: mul_mm.metal:406 is n_all += sel > 0; and :822 is the write-back's j < nr1, the three MUL_MAT_ID extras are in both ggml-metal.cpp and ggml-metal-ops.cpp, and MMQ_TILE_Y_K and block_q8_1_mmq appear only under ggml-cuda at b11081. The conclusion is unchanged. What was wrong was the provenance, which the file now states at the top: read on master, re-verified at the pin, line numbers the pin's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 of 5 tasks
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
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
Follow-up to #452, from the Metal host's third review note there: the two
mul_mv.metalline numbers indocs/maxusai/mmq-padding-metal-not-affected.mdwere three lines off at b11081. The reading was done on upstream master (dd266785c) while the file said it was done at the fork's pin, andmul_mm.metalandmul_mv.metaldiffer between the two.At b11081 (
161755f2) the quoted loops inkernel_mul_mv_q4_K_f32_implare at 1569 (the unconditional src0 read) and 1607 (the guarded write), not 1572 and 1610. At the pin, 1572 issc16[2] = …inside the read loop and 1610 isdst_f32[first_row + row] = sum_all;inside the write loop, as the review said.The conclusion is unchanged: the specific MMQ tail-padding defect is absent from Metal. What was wrong was the tree the numbers came from.
Changes
docs/maxusai/mmq-padding-metal-not-affected.md, one file, no code:mul_mv.metal:1572→:1569and:1610→:1607; that block's...elision becomes[...], matching the other quoted block.dd266785c, re-verified atb11081, every line number in the file is the pin's.#452's own description still carries 1572/1610 and the "read from
b11081" line. The file on main is the record.Test plan
Every check below was run against
b11081(161755f2), not master. The tree is what was wrong last time, so each line names it.mul_mv.metallines, trailing citation comments stripped, diffed againstsed -n '1569p;1607p'of the pin's file: identical, whitespace included.kernel_mul_mm_idblock diffed against the pin'smul_mm.metal: two exact contiguous runs, 542–555 and 559–560, so the block is 542–560 verbatim with 556–558 (theil0/illines) elided at the marker. The clamp comment is at 552.mul_mm.metal:406isn_all += sel > 0;, and:822is the write-back'sfor (short j = sgitg; j < nr1; j += 4) {.ggml_metal_op_mul_mat_id_extra_{tpe,ids,amax}each appear in bothggml-metal.cppandggml-metal-ops.cpp;MMQ_TILE_Y_Kandblock_q8_1_mmqappear only underggml/src/ggml-cuda/.check_source_paths.py --changed-since origin/main(every referenced file resolves), name scan.ai-server/mlx-cuda🤖 Generated with Claude Code