[REFACTOR][IR] Unify StructInfo and Type#19853
Conversation
The Relax dependent type refactor needs the helper and analysis surfaces to use the consolidated type vocabulary instead of keeping StructInfo compatibility shims around. This removes the remaining renamed helper wrappers, updates the public analysis and transform entry points to Type names, and tightens final stale-name cleanup in the nested-message and analysis internals.
There was a problem hiding this comment.
Code Review
This pull request refactors the Relax dialect in TVM by replacing the StructInfo concept with a unified, richer dependent Type system across C++ source files, headers, Python bindings, and documentation. The review feedback highlights several critical and medium-severity issues, including potential null pointer dereferences in dependent_type.cc, type_functor.cc, type.h, and distributed/type.cc due to missing definition checks. Additionally, there are compilation risks identified in dependent_type.cc regarding the use of the potentially undefined TVM_FFI_ICHECK_GE macro, which should be replaced with the standard TVM_FFI_ICHECK macro.
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.
| ffi::Optional<ShapeType> shape_ty = MatchType<ShapeType>(shape); | ||
| TVM_FFI_ICHECK(shape_ty) << "We expect shape to contain pre-set shape type"; | ||
| TVM_FFI_ICHECK(shape.defined()) << "Must provide a shape in this constructor"; |
There was a problem hiding this comment.
Critical Bug: Potential null pointer dereference. MatchType<ShapeType>(shape) is called on shape before checking if shape is defined on line 121. If shape is null (undefined), MatchType will dereference it and cause a segmentation fault.
Please check shape.defined() first before calling MatchType.
| ffi::Optional<ShapeType> shape_ty = MatchType<ShapeType>(shape); | |
| TVM_FFI_ICHECK(shape_ty) << "We expect shape to contain pre-set shape type"; | |
| TVM_FFI_ICHECK(shape.defined()) << "Must provide a shape in this constructor"; | |
| TVM_FFI_ICHECK(shape.defined()) << "Must provide a shape in this constructor"; | |
| ffi::Optional<ShapeType> shape_ty = MatchType<ShapeType>(shape); | |
| TVM_FFI_ICHECK(shape_ty) << "We expect shape to contain pre-set shape type"; |
| } else { | ||
| TVM_FFI_ICHECK(ret.defined()) << "FuncType that contains params must contain ret"; | ||
| return FuncType(params.value(), ret, op->purity, op->span); | ||
| } |
There was a problem hiding this comment.
Critical Bug: Potential empty optional dereference. If op->params is not defined (e.g., for an opaque function type), params remains undefined (std::nullopt). If the return type ret changes, the code enters the else branch and calls params.value(), which will crash.
We should check if params is defined, and if not, construct an opaque function type using FuncType::OpaqueFunc.
} else {
TVM_FFI_ICHECK(ret.defined()) << "FuncType must contain ret";
if (params.defined()) {
return FuncType(params.value(), ret, op->purity, op->span);
} else {
return FuncType::OpaqueFunc(ret, op->purity, op->span);
}
}| if (!shape.defined()) return {}; | ||
| ShapeType shape_ty = Downcast<ShapeType>(this->shape.value()->ty); | ||
| return shape_ty->values; |
There was a problem hiding this comment.
Defensive Programming: Potential null pointer dereference. If this->shape.value()->ty is not defined (null), Downcast<ShapeType> will fail or crash. We should defensively check if ty is defined and is of type ShapeTypeNode before accessing its values.
if (!shape.defined() || !shape.value()->ty.defined()) return {};
if (const auto* shape_ty = shape.value()->ty.as<ShapeTypeNode>()) {
return shape_ty->values;
}
return {};|
|
||
| ShapeType::ShapeType(int ndim, Span span) { | ||
| ffi::ObjectPtr<ShapeTypeNode> n = ffi::make_object<ShapeTypeNode>(); | ||
| TVM_FFI_ICHECK_GE(ndim, -1) << "ndim of ShapeType must be >= -1, but got " << ndim; |
There was a problem hiding this comment.
Compilation Risk: TVM_FFI_ICHECK_GE might not be defined in the TVM FFI headers. It is safer and more consistent to use the standard TVM_FFI_ICHECK macro with a comparison operator.
| TVM_FFI_ICHECK_GE(ndim, -1) << "ndim of ShapeType must be >= -1, but got " << ndim; | |
| TVM_FFI_ICHECK(ndim >= -1) << "ndim of ShapeType must be >= -1, but got " << ndim; |
|
|
||
| TensorType::TensorType(DataType dtype, int ndim, ffi::Optional<VDevice> vdevice, Span span) { | ||
| ffi::ObjectPtr<TensorTypeNode> n = ffi::make_object<TensorTypeNode>(); | ||
| TVM_FFI_ICHECK_GE(ndim, -1) << "ndim of TensorType must be >= -1, but got " << ndim; |
There was a problem hiding this comment.
Compilation Risk: TVM_FFI_ICHECK_GE might not be defined in the TVM FFI headers. It is safer and more consistent to use the standard TVM_FFI_ICHECK macro with a comparison operator.
| TVM_FFI_ICHECK_GE(ndim, -1) << "ndim of TensorType must be >= -1, but got " << ndim; | |
| TVM_FFI_ICHECK(ndim >= -1) << "ndim of TensorType must be >= -1, but got " << ndim; |
| TVM_FFI_CHECK_EQ(device_mesh->shape.size(), placement->dim_specs.size(), ValueError) | ||
| << "The device mesh and placement must have the same dimension size"; |
There was a problem hiding this comment.
Defensive Programming: Potential null pointer dereference. If device_mesh or placement is null (not defined), dereferencing them will cause a segmentation fault. We should defensively check if they are defined first.
TVM_FFI_CHECK(device_mesh.defined(), ValueError) << "device_mesh must be defined";
TVM_FFI_CHECK(placement.defined(), ValueError) << "placement must be defined";
TVM_FFI_CHECK_EQ(device_mesh->shape.size(), placement->dim_specs.size(), ValueError)
<< "The device mesh and placement must have the same dimension size";
Summary
Validation
Note: this build directory emits
ninja: warning: premature end of file; recovering; the final build still completed successfully after broad rebuild.