ggml-cuda: better vram to lds loading pipeline in load_tiles_q8_0 - #21698
ggml-cuda: better vram to lds loading pipeline in load_tiles_q8_0#21698iacopPBK wants to merge 3 commits into
Conversation
|
Small batches almost the same, huge boost >32
|
|
Hmm this is wierd. really the compiler should be optimizing this to MUBUF instructions to load directly from global memory into lds, and not use any registers for this at all. Intuatively i would say that your change makes it mutch harder for the compiler to optimize this in. Have you looked at the output instructions, i suspect the compiler is not managing in either version for some reason. |
|
yeah i agree, its like mi50 cant optimize that. I will check before / after instructions for vec dot q8 kernel. dont know if it is the compiler or the gpu itself 😆 |
|
yeah so i would expect it to use buffer_load_dword with the lds destination parameter, its not doing it in either version mmq-instance-q8_0-hip-amdgcn-amd-amdhsa-gfx908_pr.txt |
|
Cool! It must be found a way to hint those direct loads then, and it will be better. Regarding the oom data masking to 0, do you think is useful or just redundant? |
|
Do you have some insights how much of the perf gains are due to loop optimization (hoisting invariants outside the loop + replacing if's with ternaries) vs. splitting the loop? Register pressure is quite high, though typically not at loading the tiles into SMEM in MMQ (at least on CUDA).
In CUDA, this would indicate that the compiler does not materialize the declared register arrays, but instead merges the loops again, hence my question. Note I have not taken a look at PTX/SASS for the proposed changes. |
|
I'm reading the vega 7nm manual and i'm concerned about the direct load to lsd feasibility: This part is saying that it truncates the last two bit of the address, so it always rounds to 4byte memory alignment. Q8_0 block is 34bytes so basically never aligned. Maybe this is one of the reasons compiler dont emits buffer dwords to lds. Another thing that probably the manual is telling me is: That i can't do the bitwise OR and shitfs implemented in the get_int_b2 because it needs ALU (that require vgprs). So at this point i'm not sure we are doing any direct load to LDS at all in every GPU with this function, even though i don't know how the cp.async would work here and if it supports alu ops on LDS. Maybe its just that the vega scheduler can't realize that the instructions can be compacted, while the 6800xt one can. |
The speedup is purely related to the split loop. Keeping that stuff outside the loop didn't change anything, it only looked "cleaner" to me. I'm not sure about the ternary vs ifs cause i replaced them directly. |
|
As I said before: please take a look at the code I implemented in the tile and vector FA kernels. Many of the same optimizations I implemented there will also apply to MMQ. Even a simple patch like JohannesGaessler@2bb9ae0 outperforms this PR in my testing:
Also the code you've submitted is just too convoluted and complex. I would really not want to merge and maintain it if there is an alternative. |
|
I added a threads_per_row selector. With these threads_per_row i tried to split the loops again as the first commit and there was no speedup. No way to use memcpy here because the loads are 2byte aligned so no vectorization possible. @JohannesGaessler do you think there is a better way to assign the threads instead of this empirical approach? If this kind of selection is ok we can expand it for other archs with some tests. 16 is the best number for vega because it keeps the small batches fast and at long ctx (tested pp32184 -d0,32184), 8 and 16 are almost identical, so runtime check can't help. |
|
I don't think there is a better way to do it than to just benchmark different values and pick the best one. Please be aware that there are concurrent efforts #21849 to change how kernel orchestration is done, any changes to the number of threads used for loading should be made to fit that pattern. |
Fewer threads-per-row load more rows per warp with better HBM coalescing on wave64. Prefill q8 gain on gfx906, bit-exact (greedy md5 unchanged): Qwen3-14B-Q8 1-GPU pp512 271 -> 409 (+51%) Qwen3.6-27B-Q8 4-GPU pp512 425 -> 575 (+35%) Origin: iacopPBK's open upstream PR ggml-org#21698 and fork PR #1. Re-applied onto b10064's rewritten MMQ and swept: 8 is the fastest value and the correctness floor - below 8 under-covers QI8_0 and the tile diverges. The upstream PR used 16, correct but ~27% slower than 8 on gfx906. gfx906 (GCN5) only, all other archs unchanged.




Overview
This pr rewrites some portions of the load_tile_q8_0 function, to get a good performance improvement with the MI50.
It splits the loading tiles loop in two phases, from vram to registers, then from registers to LDS, instead of doing both operations in a single loop. The scale loading loop is like upstream. Another thing is that when need_check case happens now it masks to zero the oob data writes, not a big deal but maybe cleaner.
Quick bench results:
Mi50 is much faster with this code, while rx6800xt shows no regression.
Tried different models (qwen3/3.5 4b, 30b, mistral) and it works on both gpus, answers are correct at all ctx lenghts.
For old gpus the only concern that comes in my mind is that maybe now i'm using more VGPRS. I looked at the compilation statistics and there is not any net increase in vgprs...i didnt lose occupancy on any kernel variant.
I didn't test the tensor path, no idea if it works, but i think it should behave like upstream (there are no changes in the splitted part, just a variable rename). All the changes are commented in code.
curious to see some benchmarks of both older and newer gpus!
@JohannesGaessler what do you think ?
Requirements