[Kernel][Hardware][AMD] Add support for GGUF quantization on ROCm #10254
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can do one of these:
🚀 |
|
We have kernel tests for gguf in tests/kernels/test_gguf.py, can you enable it in |
| @@ -1,7 +1,7 @@ | |||
| // copied from https://github.com/ggerganov/llama.cpp/blob/b2899/ggml-common.h | |||
| #define QK_K 256 | |||
| #define K_QUANTS_PER_ITERATION 2 | |||
| #define WARP_SIZE 32 | |||
| #define WARP_SIZE_GGUF 32 | |||
There was a problem hiding this comment.
Do you need to change this name? It seems the vast majority of the changes in this PR are due to this rename, so would prefer to keep it
There was a problem hiding this comment.
The WARP_SIZE macro here conflicts with one defined in "cuda_compat.h" which has cross-platform utilities that this port uses. For CUDA the macro redefinition may be just fine because they are replaced by the same values. But for ROCm, in the gguf quantization kernel, some of the symbols need to be replaced by values different from that in "cuda_compat.h" while the others are kept the same, to utilize full waves on wave 64 devices. So I thought to change the macro name referenced in the quantization for clarity, though it does make quite a few mundane changes.
Another way could be to change the macro through perhaps undefine/define to the desired values at their respective places. Wouldn't need to change names in those places if we do this but the same symbol would represent different values in different places. I'm fine with either options, and would also be interested in knowing which one you would prefer.
There was a problem hiding this comment.
That's clear justification, thank you for your thoughts. I prefer the gguf name as it is more explicit, so if we must make changes with it then let's keep what you have
| @@ -467,6 +442,32 @@ def machete_prepack_B_fake(b_q_weight: torch.Tensor, | |||
| return torch.empty_like(b_q_weight, | |||
| memory_format=torch.contiguous_format) | |||
|
|
|||
| if hasattr(torch.ops._C, "ggml_dequantize"): | |||
There was a problem hiding this comment.
Leave a comment on why this can be conditional
There was a problem hiding this comment.
Originally the meta kernel was added under another conditional check, albeit checking for another op. I suppose it's so that in platforms where these kernels are not built it wouldn't run into problems loading this script. I'm not very familiar with torch's custom kernel registration, and would appreciate if you could give pointers on the proper way of handling this.
There was a problem hiding this comment.
I'm honestly not sure why but I do know that this pattern has been used before - it is a fair check to make
| @@ -1,7 +1,7 @@ | |||
| // copied from https://github.com/ggerganov/llama.cpp/blob/b2899/ggml-common.h | |||
| #define QK_K 256 | |||
| #define K_QUANTS_PER_ITERATION 2 | |||
| #define WARP_SIZE 32 | |||
| #define WARP_SIZE_GGUF 32 | |||
There was a problem hiding this comment.
That's clear justification, thank you for your thoughts. I prefer the gguf name as it is more explicit, so if we must make changes with it then let's keep what you have
|
Please merge from main to fix the CI failures. |
Signed-off-by: Maxime Fournioux <[email protected]>
This PR adds support for running GGUF models on ROCm with vLLM.