Repository navigation
ggml-cuda : deterministic radix top-k, honour GGML_PREC_F32 for F32 x F32 MUL_MAT - #292
danielhanchen wants to merge 4 commits into
Conversation
… size F32 x F32 MUL_MAT ran in exact F32 only for small batches; medium batches used TF32 tensor cores (MMF) and large ones cuBLAS tensor-op math, so results depended on the batch size even at GGML_PREC_F32. When src1 is requested at GGML_PREC_F32 ([TAG_GGML_PREC]) and the weights are F32, use the F32 matrix-vector kernel up to MMVF_MAX_BATCH_SIZE columns, else cuBLAS SGEMM with CUBLAS_DEFAULT_MATH (restored to CUBLAS_TF32_TENSOR_OP_MATH afterwards, as in solve_tri). Other paths are unchanged. test-backend-ops: MUL_MAT prec-F32 cases at NMSE 1e-10 across the MMVF / MMF / cuBLAS ranges.
…put) The radix top-k gather placed elements with atomicAdd slot counters, so both the set of tied elements at the k-th key and the order of the selected indices changed from run to run. Exact ties are common (masked or ReLU-zeroed rows), and consumers that gather rows in top-k order then accumulate in a different order every run. Each block now owns a contiguous column chunk. A count pass records the elements above and equal to the cut per chunk; the gather pass writes them at the sum of the earlier chunks' counts plus an ordered in-block prefix count. Each row holds the elements above the cut, then the ties with the lowest column indices, both in ascending column order. The shared atomic counter is gone. test-backend-ops: TOP_K cases with fewer than k positive values and the rest exactly 0.0 across the radix path.
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. |
![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: 8aef9863b3
ℹ️ 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".
| // top_k with fewer than k positive values and the rest exactly 0: the cut falls inside a long run of ties | ||
| struct test_top_k_zero_ties : public test_top_k { | ||
| test_top_k_zero_ties(std::array<int64_t, 4> ne, int k) | ||
| : test_top_k(GGML_TYPE_F32, ne, k, true) {} |
There was a problem hiding this comment.
Passing ties=true reuses test_top_k::err, which explicitly ignores the selected indices and compares only their source values and uniqueness. Consequently, the previous atomic gather passes every new zero-tie case because any distinct zero-valued indices are accepted, so these tests do not detect either nondeterministic selection or failure to choose the lowest columns. Override the comparison to check the expected index set or repeat the operation and compare outputs.
Useful? React with 👍 / 👎.
| // above-cut elements are written in ascending column order and ties take the lowest columns first, so the selected | ||
| // set and its order are reproducible (exact ties are common, e.g. masked or ReLU-zeroed rows) |
There was a problem hiding this comment.
This comment splits one sentence solely to hard-wrap it across source lines; keep the sentence on one line to follow the repository's explicit comment-formatting rule.
AGENTS.md reference: AGENTS.md:L84-L84
Useful? React with 👍 / 👎.
Makes the DeepSeek-V4.1 CUDA path deterministic and keeps F32 matmuls in F32 when the graph asks for it. Both changes are generic ggml-cuda fixes; DeepSeek-V4.1 is where they showed up.
Stacked on the minimal DeepSeek-V4.1 PR (#289,
dsv41-slim). The diff is only this change: 3 files, +201/-26.What it adds
ggml/src/ggml-cuda/top-k.cu):atomicAddslot counters, so both the set of tied elements at the k-th key and the order of the selected indices changed from run to run. Exact ties are common (masked or ReLU-zeroed rows).GGML_PREC_F32honoured for F32 x F32MUL_MATat every batch size (ggml/src/ggml-cuda/ggml-cuda.cu):GGML_PREC_F32request, the F32 matrix-vector kernel runs up toMMVF_MAX_BATCH_SIZEcolumns, else cuBLAS SGEMM withCUBLAS_DEFAULT_MATH(restored afterwards, as insolve_tri). Other paths are unchanged.test-backend-ops):TOP_Kcases with fewer than k positive values and the rest exactly 0.0;MUL_MATprec-F32 cases at NMSE 1e-10 across the MMVF / MMF / cuBLAS ranges.Verification
All results are local, on B200 (CUDA
sm_100a) and x86 CPU.test-backend-opson CUDA:TOP_K547/547,MUL_MAT2465/2465. A tie stress test (64 rows of 300,001 columns, 0, 3 and 50 distinct levels) passes, and the tie order matchestorch.topk(lowest index first).Known gaps
sm_100a). No ROCm, Metal, Vulkan, Windows or macOS validation.