Skip to content

Conversation

@jeffbolznv
Copy link
Collaborator

@jeffbolznv jeffbolznv commented Nov 21, 2025

Based on #17365. Ready for review, but Draft until that lands.

@jeffbolznv jeffbolznv marked this pull request as draft November 21, 2025 00:02
@jeffbolznv jeffbolznv mentioned this pull request Nov 21, 2025
5 tasks
@github-actions github-actions bot added testing Everything test related Vulkan Issues specific to the Vulkan backend ggml changes relating to the ggml tensor library for machine learning Apple Metal https://en.wikipedia.org/wiki/Metal_(API) labels Nov 21, 2025
@ggerganov
Copy link
Member

I ran the exhaustive top-k tests on my Mac and it got stuck at some point:

diff --git a/tests/test-backend-ops.cpp b/tests/test-backend-ops.cpp
index 83ba67502..035656943 100644
--- a/tests/test-backend-ops.cpp
+++ b/tests/test-backend-ops.cpp
@@ -7654,9 +7654,9 @@ static std::vector<std::unique_ptr<test_case>> make_test_cases_eval() {
     }
 
     // exhaustive top_k tests
-    //for (int i = 1; i < 9999; ++i) {
-    //    test_cases.emplace_back(new test_top_k(GGML_TYPE_F32, {i, 2, 1, 3}, rand() % i + 1));
-    //}
+    for (int i = 1; i < 9999; ++i) {
+        test_cases.emplace_back(new test_top_k(GGML_TYPE_F32, {i, 2, 1, 3}, rand() % i + 1));
+    }
 
     for (ggml_scale_mode mode : {GGML_SCALE_MODE_NEAREST, GGML_SCALE_MODE_BILINEAR, GGML_SCALE_MODE_BICUBIC}) {
         test_cases.emplace_back(new test_upscale(GGML_TYPE_F32, {512, 512, 3, 2}, 2, mode));
make -j && ./bin/test-backend-ops -o TOP_K -b Vulkan0

...

  TOP_K(type=f32,ne=[2699,2,1,3],k=482): OK
  TOP_K(type=f32,ne=[2700,2,1,3],k=786): OK
^C

So there could be a bug somewhere.

@jeffbolznv
Copy link
Collaborator Author

Yes, there was a bug in the pipeline selection. I'm also looking into a radix-ish implementation to see how the performance is.

Each pass launches workgroups that each sort 2^N elements (where N is usually 7-10)
and discards all but the top K. Repeat until only K are left. And there's a fast
path when K==1 to just find the max value rather than sorting.
@jeffbolznv jeffbolznv marked this pull request as ready for review November 25, 2025 14:18
@jeffbolznv
Copy link
Collaborator Author

Rebased, this is ready for review.

@0cc4m 0cc4m merged commit 879d673 into ggml-org:master Nov 26, 2025
126 of 134 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Apple Metal https://en.wikipedia.org/wiki/Metal_(API) ggml changes relating to the ggml tensor library for machine learning testing Everything test related Vulkan Issues specific to the Vulkan backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants