cuda : Add conv2d Implicit GEMM#15805
Conversation
…es for 2D convolution
github-actions
Bot
added
testing
|
Why are you adding a new ggml op? |
Because of #15669 (comment) |
|
I think the implementation of implicit gemm can directly use ggml_conv2d_direct. There's really no need to provide so many conv2d functions. |
I can reuse ggml_conv2d_direct. TBH it is not a very good or intuitive name (the best one, ggml_conv_2d, is already occupied). I do wish it has an additional argument (ggml_conv_2d should carry in the beginning) for what method implemented. |
|
If the performance of implicit gemm is on par with or even better than that of im2col + gemm, I think ggml_conv_2d can also adopt the implementation of implicit gemm. |
|
What should be done regarding For kernel selection, please take a look at how e.g. |
JohannesGaessler
left a comment
JohannesGaessler
left a comment
There was a problem hiding this comment.
For this PR, try removing the current conv2d kernel and replacing it with this one. Chances are it will be universally faster since it uses shared memory and has (unless I misread the code) coalesced memory accesses. I'll test the performance using a P40, RTX 3090, and RTX 4090 for NVIDIA and an RX 6800 and Mi 50 for AMD.
| #include "convert.cuh" | ||
|
|
||
| typedef struct{ | ||
| unsigned int n; //batch szie |
There was a problem hiding this comment.
| unsigned int n; //batch szie | |
| unsigned int n; //batch size |
|
|
||
| typedef struct{ | ||
| unsigned int n; //batch szie | ||
| unsigned int c; //channel number |
There was a problem hiding this comment.
Change to either "channel index" or "number of channels" depending on which this is.
| int threadz = 1; // threadz number per block | ||
| dim3 thblock(threadx, thready, threadz); | ||
| dim3 grid(blockx, blocky, blockz); | ||
| int smem_size = 24 * 1024; |
There was a problem hiding this comment.
On some CUDA architectures shared memory comes out of the L1 cache so it at all possible you should reserve only as much as will actually be used.
| float * __restrict__ output, | ||
| const param_t param) { | ||
|
|
||
| extern __shared__ __align__(16 * 1024) char smem[]; |
There was a problem hiding this comment.
What is the purpose of __align__ here?
There was a problem hiding this comment.
removed, no difference in performance
| for (int i = 0; i < 4; ++i) | ||
| { |
There was a problem hiding this comment.
| for (int i = 0; i < 4; ++i) | |
| { | |
| for (int i = 0; i < 4; ++i) { |
There was a problem hiding this comment.
Done. corrected styles in all places
There was a problem hiding this comment.
Thanks, @JohannesGaessler, for taking time to review. I agree with your idea as to kernel selection behind the scenes. Indeed, no single kernel is optimal for input and filter shapes. That's why cudnn provide all kinds of them for user to choose. Previously I am not sure if selecting kernels is possible and I 'll look into FLASH_ATTN_EXT example (thanks again).
Now #15813 is adding tensor support with shared mem, I don't want to step over. This PR will be in hold for now. I may contribute to the current conv_2d_direct once tensor code is merged.
There was a problem hiding this comment.
Even if there is a kernel with tensor core support a good kernel without tensor cores would still be extremely useful. P40s and Mi50s are very cheap options for 24/32 GB VRAM but they lack tensor cores. And from a ggml perspective it's much easier to squeeze out more performance than it is to compress the weights (without affecting quality).
There was a problem hiding this comment.
Speaking of P40s, you should be careful with FP16 arithmetic since that is massively gimped on Pascal. You can use the macro FAST_FP16_AVAILABLE to check whether FP16 would be fast and use FP32 as a workaround if not. You can look at e.g. mmvf.cu for an example.
There was a problem hiding this comment.
Will look into it. Thanks.
…ncy; update parameter comments and remove unused code
…test for implicit convolution
|
erroneous numbers. old numbersTook it for a small test drive in sd.cpp for VAE decoding: 768x1024 sd1 fp16 vae:
The resulting images look correct. For some reason implicitgemm is as fast as the current direct implementation. im2col+mat_muldirectimplicitgemm |
@Green-Sky, thanks for giving it a try-out. I am a bit puzzled by the results.
|
diff --git a/ggml_extend.hpp b/ggml_extend.hpp
index 560d2861..36048bcc 100644
--- a/ggml_extend.hpp
+++ b/ggml_extend.hpp
@@ -851,7 +851,7 @@ __STATIC_INLINE__ struct ggml_tensor* ggml_nn_conv_2d_direct(struct ggml_context
int p1 = 0,
int d0 = 1,
int d1 = 1) {
- x = ggml_conv_2d_direct(ctx, w, x, s0, s1, p0, p1, d0, d1);
+ x = ggml_conv_2d_implicitgemm(ctx, w, x, s0, s1, p0, p1, d0, d1);
if (b != NULL) {
b = ggml_reshape_4d(ctx, b, 1, 1, b->ne[0], 1);
// b = ggml_repeat(ctx, b, x);
Yes, I first thought I did something wrong, but it is only one call in sd.cpp (see diff) and I looked at your code and it also looks like the OP is properly emitted ... Giving it another try, just to make sure I really did nothing wrong here (: . update: ok, getting (way) better values now. I do not know what I did wrong before, I made sure to recompile a couple of times with different settings, but it really did use the naive conv2d_direct impl. My mistake. |
|
Corrected test drive in sd.cpp for VAE decoding: 768x1024 sd1 fp16 vae:
Ignore previous numbers. Now looking more like it. Good job @bssrdf :) , sorry for the confusion. edit: added vulkan numbers from same device. (cm2) |
Thanks for the update, @Green-Sky. The sd.cpp result is consistent with what I tested in ggml backend op. I hope to improve implicit's performance, especially for Fp16. There should be plenty of low hanging fruits there. |
Hi, there are a few optimizations used in Vulkan: mininizing branch divergence, reducing the number of modulo/div computations with shuffles. Later @jeffbolznv further optimized it by using constant int divs and optimized the blocktile sizes to common inputs. Although the GLSL code is not too hard to read, I added the CUDA translation of the GLSL shader in a new PR: #16088. It also introduces some bank conflict reduction, and vectorized shmem loads so it is now faster than Vulkan on large inputs. |
@etasnadi, thanks for working on the cuda backend. Yes, with these optimizations in place, cuda should be on par with vulkan if not faster. Great job! |
|
Now #16088 is a better implementation. |
I see. |
|
I just noticed the discussion. Think the changes from #19165 will likely fix the issue, but a type check would still be necessary to add for to the properties in order to distinguish same-sized types. |
|
The tests are passing again at top of tree. |
That's great. Will merge the latest and retest. Thanks, @jeffbolznv and @ggerganov. |
|
Now all test cases passed. |
|
So, how are we looking? How is HIP doing? |
I don't know how to resolve the HIP build error. Anyone who could lend a hand will be appreciated. |
|
I've been able to fix the HIP build issue. Apparently the compiler doesn't read My solution has been to add a By fixing this I've been able to compile the program and run it without any issue. |
@daniandtheweb, thanks for your help on this issue. Unfortunately adding |
|
May I ask how to compile conv2d-implicit on HIP? I just checkout bssrdf:conv2d-implicit and don't see compile error on my hip windows 6.4.2. Based on my knowledge, hipFuncSetAttribute needs const void* as function type, so CUDA_SET_SHARED_MEMORY_LIMIT needs to be modified to support hip, or you can just get rid of the macro and directly call cudaFuncSetAttribute like fattn-mma-f16.cuh |
Hi, if there is no compile error, can you try it with HIP backends and see if it works? The tensor core path is not meant for non-NVIDIA devices but the other path using cuda core may work. Sorry, I don't know much about HIP. |
I don't know if conv2d-implicit.cu is compiled as there is no conv2d-implicit.obj file, looks like that hip is disabled in conv2d-implicit.cu by !defined(GGML_USE_HIP) So I don't think the backend will have much impact to hip unless you totally replace the current cov2d with implicit gemm, I will have a try after removing !defined(GGML_USE_HIP). |
|
|
I just go through the code in conv2d-implicit.cu, cpu side also disables hip so only conv2d_implicit_cuda will be executed, all cases pass on my 7900XTX, I don't think you need to worry about CUDA_SET_SHARED_MEMORY_LIMIT as it can be compiled with unsupported code as it will never be executed. For hip support, like mmf and flash attention, you can submit cuda version first then I or someone others can adapter hip backend in the future. |
|
I just look at the main loop of implicit gemm, too many native ptx code in it which makes the kernel only works in cuda, I will suggest to change the code to adapter mma.cuh or it's hard to be ported to other platorms. |
Unfortunately I didn't use the existing mma tiles when developing the conv2d kernel. I am also not the right person to adapt this kernel to AMD devices due to knowledge gap. I think it is probably better to merge this PR and leave development for other platforms in future PRs. Thanks. |
You still need to adapter other NVIDIA GPUs like Volta, anyway I'm not the moderator, using mma.cuh is just a suggestion, the decision shall belongs to @JohannesGaessler |
|
I currently don't have the capacity to properly maintain convolution kernels, the biggest problem is that I don't have established workflows for proper QA. What exact device code implementation to go with should be left at the discretion of the person taking over the responsibility for long-term maintenance. My opinion is that using the interface in |
Thanks, @JohannesGaessler. I can commit myself to the long-term maintenance of this cuda kernel and working with anyone on bringing in HIP code for AMD devices or newer Nvidia architectures e.g. Blackwell. |
10 participants
This PR added another CUDA conv_2d op using implicit GEMM approach. It is only optimized for cuda cores and its performance is up to 10x of that of direct method currently in llama.cpp.
On a RTX4090
Comparison with im2col+gemm
Fp16 filter, Fp32 activation
Fp32 filter, Fp32 activation