[mcDNN] Support mcdnn contrib for MACA target#51
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates support for the mcDNN library into TVM, adding CMake build options, Python bindings, Relax pattern matching, and a JSON runtime for offloading operations like convolutions and attention. While the implementation is comprehensive, several critical issues were identified during the review. These include a potential compilation failure in the codegen due to incorrect usage of TensorType instead of TensorStructInfo, missing algorithm list definitions in Python, a copy-paste bug in parameter preparation, potential runtime crashes in the thread local setup and JSON runtime when handling empty layouts, and brittle hardcoded node indexing. Addressing these issues will ensure the stability and correctness of the mcDNN integration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| McDNNThreadEntry* McDNNThreadEntry::ThreadLocal(Device curr_device, bool check_exists) { | ||
| static thread_local McDNNThreadEntry inst; | ||
| auto* res = &inst; | ||
| if (check_exists) { | ||
| TVM_FFI_ICHECK(res->exists()) << "MCDNN_STATUS_NOT_INITIALIZED"; | ||
| } | ||
|
|
||
| mcStream_t stream = | ||
| static_cast<mcStream_t>(TVMFFIEnvGetStream(curr_device.device_type, curr_device.device_id)); | ||
| MCDNN_CALL(mcdnnSetStream(res->handle, stream)); | ||
| return res; | ||
| } |
There was a problem hiding this comment.
When check_exists is false (e.g., during the tvm.contrib.mcdnn.exists query) and no mcDNN-capable device is present, res->handle is nullptr. Calling mcdnnSetStream with a nullptr handle will cause a crash or runtime error, defeating the purpose of the safe existence check. Only set the stream if the handle exists.
McDNNThreadEntry* McDNNThreadEntry::ThreadLocal(Device curr_device, bool check_exists) {
static thread_local McDNNThreadEntry inst;
auto* res = &inst;
if (check_exists) {
TVM_FFI_ICHECK(res->exists()) << "MCDNN_STATUS_NOT_INITIALIZED";
}
if (res->exists()) {
mcStream_t stream =
static_cast<mcStream_t>(TVMFFIEnvGetStream(curr_device.device_type, curr_device.device_id));
MCDNN_CALL(mcdnnSetStream(res->handle, stream));
}
return res;
}| _FWD_ALGOS = [ | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_IMPLICIT_GEMM", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_IMPLICIT_PRECOMP_GEMM", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_GEMM", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_DIRECT", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_FFT", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_FFT_TILING", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_WINOGRAD", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_WINOGRAD_NONFUSED", | ||
| "MCDNN_CONVOLUTION_FWD_ALGO_COUNT", | ||
| ] |
There was a problem hiding this comment.
The lists _BWD_FILTER_ALGOS and _BWD_DATA_ALGOS are referenced in algo_to_index but are not defined anywhere in this file. This will cause a NameError at runtime when trying to resolve backward algorithms. Please define these lists at the module level.
_FWD_ALGOS = [
"MCDNN_CONVOLUTION_FWD_ALGO_IMPLICIT_GEMM",
"MCDNN_CONVOLUTION_FWD_ALGO_IMPLICIT_PRECOMP_GEMM",
"MCDNN_CONVOLUTION_FWD_ALGO_GEMM",
"MCDNN_CONVOLUTION_FWD_ALGO_DIRECT",
"MCDNN_CONVOLUTION_FWD_ALGO_FFT",
"MCDNN_CONVOLUTION_FWD_ALGO_FFT_TILING",
"MCDNN_CONVOLUTION_FWD_ALGO_WINOGRAD",
"MCDNN_CONVOLUTION_FWD_ALGO_WINOGRAD_NONFUSED",
"MCDNN_CONVOLUTION_FWD_ALGO_COUNT",
]
_BWD_FILTER_ALGOS = [
"MCDNN_CONVOLUTION_BWD_FILTER_ALGO_0",
"MCDNN_CONVOLUTION_BWD_FILTER_ALGO_1",
"MCDNN_CONVOLUTION_BWD_FILTER_ALGO_FFT",
"MCDNN_CONVOLUTION_BWD_FILTER_ALGO_3",
"MCDNN_CONVOLUTION_BWD_FILTER_ALGO_WINOGRAD_NONFUSED",
"MCDNN_CONVOLUTION_BWD_FILTER_ALGO_FFT_TILING",
]
_BWD_DATA_ALGOS = [
"MCDNN_CONVOLUTION_BWD_DATA_ALGO_0",
"MCDNN_CONVOLUTION_BWD_DATA_ALGO_1",
"MCDNN_CONVOLUTION_BWD_DATA_ALGO_FFT",
"MCDNN_CONVOLUTION_BWD_DATA_ALGO_FFT_TILING",
"MCDNN_CONVOLUTION_BWD_DATA_ALGO_WINOGRAD",
"MCDNN_CONVOLUTION_BWD_DATA_ALGO_WINOGRAD_NONFUSED",
]| xshape = np.array(x_shape, dtype=np.int32) if x_shape else None | ||
| wshape = np.array(w_shape, dtype=np.int32) if x_shape else None |
There was a problem hiding this comment.
There is a copy-paste bug where wshape is conditionally created based on x_shape instead of w_shape. This will cause a TypeError or AttributeError if x_shape is provided but w_shape is None, or vice versa. It should check w_shape instead.
| xshape = np.array(x_shape, dtype=np.int32) if x_shape else None | |
| wshape = np.array(w_shape, dtype=np.int32) if x_shape else None | |
| xshape = np.array(x_shape, dtype=np.int32) if x_shape else None | |
| wshape = np.array(w_shape, dtype=np.int32) if w_shape else None |
| std::string layout = std::string(node.GetAttr<ffi::String>("out_layout")); | ||
| int dims = layout.size() - 2; // remove O and I dims |
There was a problem hiding this comment.
In TVM Relax, out_layout is optional and defaults to an empty string "". If it is empty, the output layout defaults to data_layout. If not handled, layout will be empty, leading to a negative dimension size (dims = -2) and a runtime crash with Unsupported layout: . Fall back to data_layout if out_layout is empty.
std::string layout = std::string(node.GetAttr<ffi::String>("out_layout"));
if (layout.empty()) {
layout = std::string(node.GetAttr<ffi::String>("data_layout"));
}
int dims = layout.size() - 2; // remove O and I dims| auto q_shape = | ||
| root_call->args[0]->ty.as_or_throw<TensorType>()->shape.value().as_or_throw<ShapeExpr>(); | ||
| auto k_shape = | ||
| root_call->args[1]->ty.as_or_throw<TensorType>()->shape.value().as_or_throw<ShapeExpr>(); | ||
| auto v_shape = | ||
| root_call->args[2]->ty.as_or_throw<TensorType>()->shape.value().as_or_throw<ShapeExpr>(); |
There was a problem hiding this comment.
In TVM, TensorType does not have a shape field. Attempting to access shape on TensorType will fail to compile. In TVM Relax, the standard and correct way to retrieve the shape of a tensor is through its struct_info (specifically TensorStructInfo).
| auto q_shape = | |
| root_call->args[0]->ty.as_or_throw<TensorType>()->shape.value().as_or_throw<ShapeExpr>(); | |
| auto k_shape = | |
| root_call->args[1]->ty.as_or_throw<TensorType>()->shape.value().as_or_throw<ShapeExpr>(); | |
| auto v_shape = | |
| root_call->args[2]->ty.as_or_throw<TensorType>()->shape.value().as_or_throw<ShapeExpr>(); | |
| auto q_shape = root_call->args[0] | |
| ->struct_info.as_or_throw<TensorStructInfo>() | |
| ->shape.value() | |
| .as_or_throw<ShapeExpr>(); | |
| auto k_shape = root_call->args[1] | |
| ->struct_info.as_or_throw<TensorStructInfo>() | |
| ->shape.value() | |
| .as_or_throw<ShapeExpr>(); | |
| auto v_shape = root_call->args[2] | |
| ->struct_info.as_or_throw<TensorStructInfo>() | |
| ->shape.value() | |
| .as_or_throw<ShapeExpr>(); |
| std::vector<int> input_dims, kernel_dims, output_dims; | ||
| auto input_node = nodes_[0]; | ||
| auto input_shapes = input_node.GetOpShape()[0]; | ||
| auto kernel_shapes = nodes_[1].GetOpShape()[0]; | ||
| auto output_shapes = node.GetOpShape()[0]; |
There was a problem hiding this comment.
Hardcoding indices 0 and 1 to fetch the input and kernel nodes from nodes_ is brittle and can easily break if the node order changes or if other nodes are present. It is much safer and more robust to use node.GetInputs() to resolve the correct nodes.
std::vector<int> input_dims, kernel_dims, output_dims;
auto input_node = nodes_[EntryID(node.GetInputs()[0])];
auto kernel_node = nodes_[EntryID(node.GetInputs()[1])];
auto input_shapes = input_node.GetOpShape()[0];
auto kernel_shapes = kernel_node.GetOpShape()[0];
auto output_shapes = node.GetOpShape()[0];
USE_MCDNNto enable compile with mcdnn;tvm.contrib.mcdnn.*to call mcdnn library;