Apply universal gemm to bwd_weight_cshuffle operator #1658
Conversation
mozga-amd
force-pushed
the
mozga-amd/universal_gemm_weight
branch
from
118fd3f to
932e6a0
Compare
example/20_grouped_conv_bwd_weight/grouped_conv_bwd_weight_xdl_bf16.cpp
Outdated
Show resolved
Hide resolved
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Outdated
Show resolved
Hide resolved
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @mozga-amd please remove merged groups support for this kernel. Then please update instances. After that we can review it again
mozga-amd
force-pushed
the
mozga-amd/universal_gemm_weight
branch
from
932e6a0 to
860433e
Compare
mozga-amd
requested review from
junliume,
illsilin,
carlushuang,
qianfengz,
aosewski,
poyenc,
geyyer,
andriy-ca and
a team
as code owners
There was a problem hiding this comment.
I added a few comment. Please focus firstly on:
Regarding API changes, we have two solutions but please make some local measurements:
-
If gridwise gemm v3 is better for each case can we move BlkGemmPipeSched and BlgGemmPipVer at the end and keep api consistent?
-
If gridwise gemm v3 is better but not for each, then can we restore previous implementation and copy your new DeviceGroupedConvBwdWeight_Xdl_CShuffle as DeviceGroupedConvBwdWeight_Xdl_CShuffleV3?
example/20_grouped_conv_bwd_weight/grouped_conv_bwd_weight_xdl_bf16.cpp
Outdated
Show resolved
Hide resolved
| ck::BlockGemmPipelineVersion::v1, // BlkGemmPipelineVer | ||
| ComputeTypeA, // ComputeTypeA | ||
| ComputeTypeB>; // ComputeTypeB | ||
| // clang-format on |
There was a problem hiding this comment.
Regarding API changes, we have two solutions but please make some local measurements:
- If gridwise gemm v3 is better for each case can we move BlkGemmPipeSched and BlgGemmPipVer at the end and keep api consistent?
- If gridwise gemm v3 is better but not for each, then can we restore previous implementation and copy your new DeviceGroupedConvBwdWeight_Xdl_CShuffle as DeviceGroupedConvBwdWeight_Xdl_CShuffleV3?
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Outdated
Show resolved
Hide resolved
| const index_t K, | ||
| const std::array<index_t, NDimSpatial + 3>& output_strides) | ||
| { | ||
| const index_t BatchStride = output_strides[0]; |
There was a problem hiding this comment.
For, out, in and wei. Can we add some condintion if (NumGroupsToMerge == 1) {/* Create descriptor in typical way (like in v1) ?} Or if possible use transform_v1 in device_op? I think it could inpact on performance
...ary/include/ck/library/tensor_operation_instance/gpu/grouped_convolution_backward_weight.hpp
Show resolved
Hide resolved
test/grouped_convnd_bwd_weight/test_grouped_convnd_bwd_weight_interface_xdl.cpp
Outdated
Show resolved
Hide resolved
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Outdated
Show resolved
Hide resolved
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Outdated
Show resolved
Hide resolved
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Outdated
Show resolved
Hide resolved
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Show resolved
Hide resolved
include/ck/tensor_operation/gpu/device/impl/device_grouped_conv_bwd_weight_xdl_cshuffle.hpp
Show resolved
Hide resolved
No description provided.