gemini-code-assist[bot] commented on code in PR #19875:
URL: https://github.com/apache/tvm/pull/19875#discussion_r3461546596
##########
src/backend/hexagon/codegen/llvm/codegen_hexagon.cc:
##########
@@ -66,6 +66,11 @@
namespace tvm {
namespace codegen {
+TVM_FFI_INLINE int GetVectorBytes(const PrimType& dtype) {
+ TVM_FFI_ICHECK(dtype.IsFixedLengthVector() || dtype.IsScalar());
+ return dtype.bits() * dtype.lanes() / 8;
+}
Review Comment:
In `GetVectorBytes`, using `dtype.bits() * dtype.lanes() / 8` can return `0`
for sub-byte types (such as 4-bit integers or booleans). This will lead to a
division-by-zero error in `VectorLookupLoad` where `native_vector_bytes /
GetVectorBytes(buffer_type)` is calculated. Using `(dtype.bits() *
dtype.lanes() + 7) / 8` safely computes the byte size and prevents this
potential compiler crash.
```c
TVM_FFI_INLINE int GetVectorBytes(const PrimType& dtype) {
TVM_FFI_ICHECK(dtype.IsFixedLengthVector() || dtype.IsScalar());
return (dtype.bits() * dtype.lanes() + 7) / 8;
}
```
##########
python/tvm/ir/expr.py:
##########
@@ -43,7 +43,10 @@ class PrimExpr(BaseExpr):
optimizations and integer analysis.
"""
- dtype: str
+ @property
+ def dtype(self):
+ """Return the runtime dtype represented by this expression's
PrimType."""
+ return self.ty.dtype
Review Comment:
The `dtype` property on `PrimExpr` is implemented as `return self.ty.dtype`.
However, `self.ty` can be `None` or a `PointerType` (which does not have a
`dtype` attribute). This will raise an `AttributeError` when accessing `.dtype`
on handle variables or un-typed expressions. Adding safety checks to handle
`None` and `PointerType` (returning `"handle"` for pointers) ensures backward
compatibility and robustness.
```suggestion
@property
def dtype(self):
"""Return the runtime dtype represented by this expression's
PrimType."""
if self.ty is None:
return None
if hasattr(self.ty, "dtype"):
return self.ty.dtype
return "handle"
```
##########
include/tvm/topi/detail/broadcast.h:
##########
@@ -56,15 +58,15 @@ inline BroadcastHelper BroadcastShape(const
tvm::ffi::Array<tvm::PrimExpr>& shap
tvm::PrimExpr one(1);
int i;
- auto cast_if_needed = [](DataType to_type, PrimExpr expr) {
- return to_type != expr.dtype() ? cast(to_type, expr) : expr;
+ auto cast_if_needed = [](PrimType to_type, PrimExpr expr) {
+ return to_type->dtype == expr.ty()->dtype ? expr : cast(to_type, expr);
};
Review Comment:
Since `PrimType` has `operator==` defined, the check `to_type->dtype ==
expr.ty()->dtype` can be simplified to `to_type == expr.ty()`.
```suggestion
auto cast_if_needed = [](PrimType to_type, PrimExpr expr) {
return to_type == expr.ty() ? expr : cast(to_type, expr);
};
```
##########
include/tvm/topi/detail/broadcast.h:
##########
@@ -42,10 +42,12 @@ struct BroadcastHelper {
std::deque<tvm::tirx::Var> vars2;
};
-static inline DataType CommonType(DataType type1, DataType type2) {
- TVM_FFI_ICHECK(type1.is_scalar() && type2.is_scalar());
+static inline PrimType CommonType(const PrimType& type1, const PrimType&
type2) {
+ TVM_FFI_ICHECK(!type1.IsScalableVector() && !type2.IsScalableVector());
+ TVM_FFI_ICHECK_EQ(type1.lanes(), 1);
+ TVM_FFI_ICHECK_EQ(type2.lanes(), 1);
Review Comment:
The checks to ensure `type1` and `type2` are scalar can be simplified and
made more robust by using the newly introduced `IsScalar()` helper method on
`PrimType`.
```suggestion
TVM_FFI_ICHECK(type1.IsScalar() && type2.IsScalar());
```
##########
src/arith/const_fold.h:
##########
@@ -72,18 +72,29 @@ inline ffi::Optional<PrimExpr> TryConstFold(PrimExpr a);
* \param type The type to represent index.
* \return the checked result.
*/
-inline bool IsIndexType(const DataType& type) {
- return type.is_int() && !type.is_scalable_or_fixed_length_vector() &&
- (type.bits() == 32 || type.bits() == 64);
+inline bool IsIndexType(DLDataType type) {
+ return type.code == static_cast<uint8_t>(DLDataTypeCode::kDLInt) &&
+ (type.bits == 32 || type.bits == 64) && type.lanes == 1;
+}
+
+inline bool IsIndexTypedExpr(const PrimExprNode* expr) {
+ TVM_FFI_DCHECK(expr != nullptr);
+ TVM_FFI_DCHECK(expr->BaseExprNode::ty.defined());
+ const auto* prim_ty = expr->BaseExprNode::ty.as<PrimTypeNode>();
+ TVM_FFI_DCHECK(prim_ty != nullptr);
+ return IsIndexType(prim_ty->dtype);
Review Comment:
The manual checks on `expr->BaseExprNode::ty` are redundant because
`PrimExprNode::ty()` already implements these checks and returns `PrimType`. We
can simplify this to a single call to `expr->ty()`.
```suggestion
return IsIndexType(expr->ty()->dtype);
```
##########
src/s_tir/transform/bound_checker.cc:
##########
@@ -175,7 +177,9 @@ class BoundChecker : public StmtExprMutator {
}
bool IsValidScalar(const PrimExpr& expr) const {
- return expr.defined() && expr.dtype().is_scalar();
+ if (!expr.defined()) return false;
Review Comment:
The check `!ty.IsFixedLengthVector() && !ty.IsScalableVector()` can be
simplified using the `IsScalar()` helper method on `PrimType`.
```c
return expr.defined() && expr.ty().IsScalar();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
