gemini-code-assist[bot] commented on code in PR #668:
URL: https://github.com/apache/tvm-ffi/pull/668#discussion_r3590246321
##########
python/tvm_ffi/_dtype.py:
##########
@@ -288,6 +289,72 @@ def lanes(self) -> int:
"""
return self._tvm_ffi_dtype.lanes
+ @property
+ def is_bool(self) -> builtins.bool:
+ """Whether this dtype stores boolean values."""
+ return self.type_code == DataTypeCode.BOOL
+
+ @property
+ def is_integer(self) -> builtins.bool:
+ """Whether this dtype stores signed or unsigned integer values."""
+ return self.type_code in {DataTypeCode.INT, DataTypeCode.UINT}
Review Comment:
Using a set literal `{DataTypeCode.INT, DataTypeCode.UINT}` inside the
`is_integer` property creates a new set object on every single access. Since
this property is frequently accessed in FFI operations, we can optimize it by
using direct boolean comparison, which avoids any object allocation.
```suggestion
@property
def is_integer(self) -> builtins.bool:
"""Whether this dtype stores signed or unsigned integer values."""
return self.type_code == DataTypeCode.INT or self.type_code ==
DataTypeCode.UINT
```
##########
python/tvm_ffi/_dtype.py:
##########
@@ -288,6 +289,72 @@ def lanes(self) -> int:
"""
return self._tvm_ffi_dtype.lanes
+ @property
+ def is_bool(self) -> builtins.bool:
+ """Whether this dtype stores boolean values."""
+ return self.type_code == DataTypeCode.BOOL
+
+ @property
+ def is_integer(self) -> builtins.bool:
+ """Whether this dtype stores signed or unsigned integer values."""
+ return self.type_code in {DataTypeCode.INT, DataTypeCode.UINT}
+
+ @property
+ def is_float(self) -> builtins.bool:
+ """Whether this dtype stores floating-point values."""
+ return self.type_code in {
+ DataTypeCode.FLOAT,
+ DataTypeCode.BFLOAT,
+ DataTypeCode.Float8E3M4,
+ DataTypeCode.Float8E4M3,
+ DataTypeCode.Float8E4M3B11FNUZ,
+ DataTypeCode.Float8E4M3FN,
+ DataTypeCode.Float8E4M3FNUZ,
+ DataTypeCode.Float8E5M2,
+ DataTypeCode.Float8E5M2FNUZ,
+ DataTypeCode.Float8E8M0FNU,
+ DataTypeCode.Float6E2M3FN,
+ DataTypeCode.Float6E3M2FN,
+ DataTypeCode.Float4E2M1FN,
+ }
Review Comment:
Defining a set literal inside the `is_float` property causes a new set
object to be allocated and initialized on every property access. To improve
performance and avoid unnecessary garbage collection overhead, we should define
the set of float type codes as a class-level constant `_FLOAT_CODES`.
```suggestion
_FLOAT_CODES: ClassVar[set[int]] = {
DataTypeCode.FLOAT,
DataTypeCode.BFLOAT,
DataTypeCode.Float8E3M4,
DataTypeCode.Float8E4M3,
DataTypeCode.Float8E4M3B11FNUZ,
DataTypeCode.Float8E4M3FN,
DataTypeCode.Float8E4M3FNUZ,
DataTypeCode.Float8E5M2,
DataTypeCode.Float8E5M2FNUZ,
DataTypeCode.Float8E8M0FNU,
DataTypeCode.Float6E2M3FN,
DataTypeCode.Float6E3M2FN,
DataTypeCode.Float4E2M1FN,
}
@property
def is_float(self) -> builtins.bool:
"""Whether this dtype stores floating-point values."""
return self.type_code in self._FLOAT_CODES
```
##########
include/tvm/ffi/dtype.h:
##########
@@ -138,6 +138,160 @@ inline String DLDataTypeToString(DLDataType dtype) {
return TypeTraits<String>::MoveFromAnyAfterCheck(&out);
}
+/*!
+ * \brief Check whether a DLDataType is a floating-point type.
+ * \param dtype The DLDataType to check.
+ * \return True if dtype is a floating-point type, false otherwise.
+ */
+inline bool DTypeIsFloat(DLDataType dtype) {
+ switch (static_cast<int>(dtype.code)) {
+ case kDLFloat:
+ case kDLBfloat:
+ case kDLFloat8_e3m4:
+ case kDLFloat8_e4m3:
+ case kDLFloat8_e4m3b11fnuz:
+ case kDLFloat8_e4m3fn:
+ case kDLFloat8_e4m3fnuz:
+ case kDLFloat8_e5m2:
+ case kDLFloat8_e5m2fnuz:
+ case kDLFloat8_e8m0fnu:
+ case kDLFloat6_e2m3fn:
+ case kDLFloat6_e3m2fn:
+ case kDLFloat4_e2m1fn:
+ return true;
+ default:
+ return false;
+ }
+ return false;
+}
Review Comment:
The `return false;` statement at the end of `DTypeIsFloat` is unreachable
because the `switch` statement already covers all cases and has a `default`
branch that returns `false`. Some compilers with strict warning settings (e.g.,
`-Wunreachable-code`) may emit warnings or errors for this. We should remove
the redundant return statement.
```c
inline bool DTypeIsFloat(DLDataType dtype) {
switch (static_cast<int>(dtype.code)) {
case kDLFloat:
case kDLBfloat:
case kDLFloat8_e3m4:
case kDLFloat8_e4m3:
case kDLFloat8_e4m3b11fnuz:
case kDLFloat8_e4m3fn:
case kDLFloat8_e4m3fnuz:
case kDLFloat8_e5m2:
case kDLFloat8_e5m2fnuz:
case kDLFloat8_e8m0fnu:
case kDLFloat6_e2m3fn:
case kDLFloat6_e3m2fn:
case kDLFloat4_e2m1fn:
return true;
default:
return false;
}
}
```
##########
include/tvm/ffi/dtype.h:
##########
@@ -138,6 +138,160 @@ inline String DLDataTypeToString(DLDataType dtype) {
return TypeTraits<String>::MoveFromAnyAfterCheck(&out);
}
+/*!
+ * \brief Check whether a DLDataType is a floating-point type.
+ * \param dtype The DLDataType to check.
+ * \return True if dtype is a floating-point type, false otherwise.
+ */
+inline bool DTypeIsFloat(DLDataType dtype) {
+ switch (static_cast<int>(dtype.code)) {
+ case kDLFloat:
+ case kDLBfloat:
+ case kDLFloat8_e3m4:
+ case kDLFloat8_e4m3:
+ case kDLFloat8_e4m3b11fnuz:
+ case kDLFloat8_e4m3fn:
+ case kDLFloat8_e4m3fnuz:
+ case kDLFloat8_e5m2:
+ case kDLFloat8_e5m2fnuz:
+ case kDLFloat8_e8m0fnu:
+ case kDLFloat6_e2m3fn:
+ case kDLFloat6_e3m2fn:
+ case kDLFloat4_e2m1fn:
+ return true;
+ default:
+ return false;
+ }
+ return false;
+}
+
+/*!
+ * \brief Check whether a DLDataType is an integer type.
+ * \param dtype The DLDataType to check.
+ * \return True if dtype is a signed or unsigned integer type, false otherwise.
+ */
+inline bool DTypeIsInt(DLDataType dtype) {
+ switch (static_cast<int>(dtype.code)) {
+ case kDLInt:
+ case kDLUInt:
+ return true;
+ default:
+ return false;
+ }
+ return false;
+}
Review Comment:
The `return false;` statement at the end of `DTypeIsInt` is unreachable
because the `switch` statement has a `default` branch that returns `false`. We
should remove this redundant return statement to keep the code clean and avoid
potential compiler warnings.
```c
inline bool DTypeIsInt(DLDataType dtype) {
switch (static_cast<int>(dtype.code)) {
case kDLInt:
case kDLUInt:
return true;
default:
return false;
}
}
```
--
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]
