Vulkan: Add Integer Dot Product mul_mat_vec shader for legacy quants #14903
+310
−14
Conversation
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
github-actions
bot
added
Vulkan
|
Here are performance results from my tests: AMD Radeon Pro VIIIntel A770Nvidia RTX 3090AMD RX 6800 XT |
jeffbolznv
reviewed
Jul 27, 2025
| const uint b_block_idx = (j*p.batch_stride_b + col) / QUANT_K_Q8_1 + b_offset; | ||
| cache_b_ds = vec2(data_b[b_block_idx].ds); | ||
| [[unroll]] for (uint k = 0; k < 8; k++) { | ||
| cache_b_qs[k] = data_b[b_block_idx].qs[k]; |
There was a problem hiding this comment.
You need a barrier after these shared memory stores, and either after the loads or before the stores for the next iteration.
Seems like you can cut down the loads by having the first 8 threads each do one of the iterations. And ds could just go straight to registers rather than the extra copy through shared memory.
There was a problem hiding this comment.
That's not shared memory
There was a problem hiding this comment.
Oops. Maybe it's worth loading the qs values through shared memory? If the issue is with too many small loads like you suggested, then copying through shared memory ought to help.
There was a problem hiding this comment.
Actually, I guess I can't tell if the b_block_idx value is shared between threads. So maybe this idea doesn't work.
There was a problem hiding this comment.
Another idea might be to add padding to the q8_1 struct so you can do 16B loads rather than 4B loads.
There was a problem hiding this comment.
Yeah, that might be worth it. I know the cuda backend stacks 4 q8_1 blocks in a struct for that reason.
|
I did a quick before/after on some Q4_0 models, and it looks like the quantization is pretty expensive: I don't think there's anything particularly wrong with how the quantization is implemented, it's such a small amount of work that it doesn't fill the GPU, and 5090 is just about the worst case for that. I don't have any great suggestions for what to do about this. |
|
Yeah, I also see that. We might have to pick a threshold from which using this quantize + integer dot shader path is worth it. Even without further tuning, there are definitely cases where it helps, for example batch 4 and 8 on RX 6800 XT: Master:
PR:
|
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.
Here's an initial version of an Integer Dot mul_mat_vec shader. So far it seems to improve performance with q4_1 and q5_1, but reduce it with q4_0, q5_0 and q8_0. My guess is that this is because of the 32-bit loads in q4_1 and q5_1, while the rest use 16-bit loads.
@jeffbolznv Would you mind taking a look and letting me know if I have any obvious performance issues in the shader?