tqchen commented on code in PR #16183:
URL: https://github.com/apache/tvm/pull/16183#discussion_r1409728621
##########
include/tvm/runtime/packed_func.h:
##########
@@ -547,29 +551,43 @@ class TVMPODValue_ {
// Allow automatic conversion from int to float
// This avoids errors when user pass in int from
// the frontend while the API expects a float.
- if (type_code_ == kDLInt) {
- return static_cast<double>(value_.v_int64);
+ if (auto opt = TryAsBool()) {
+ return opt.value();
+ } else if (auto opt = TryAsInt()) {
Review Comment:
We should go in the order of
- TryAsFloat, TryAsInt, TryAsBool
Prioritize the most likely ones
##########
include/tvm/runtime/packed_func.h:
##########
@@ -627,13 +645,54 @@ class TVMPODValue_ {
template <typename TObjectRef>
inline TObjectRef AsObjectRef() const;
+ std::optional<int64_t> TryAsInt() const {
+ // Helper function to reduce duplication in the variable integer
+ // conversions. This is publicly exposed, as it can be useful in
+ // specializations of PackedFuncValueConverter.
+ if (auto opt = FromBoxed<int64_t>()) {
Review Comment:
Calling convention note: it would be simpler to require that TVMArgValue and
TVMRetValue always NOT hold Boxed values. This would help to reduce the overall
cost of the codegen function handling.
That does mean when we assign Boxed Object into TVMArg and TVMRet, they
should be unboxed. I know that we can make an exception for bool for now until
we introduce a specific bool POD code.
##########
include/tvm/runtime/packed_func.h:
##########
@@ -2129,6 +2315,42 @@ struct PackedFuncValueConverter<::tvm::runtime::String> {
}
};
+template <typename T>
+struct PackedFuncValueConverter<Array<T>> {
Review Comment:
I feel automatic conversion here is a bit overkill. Recursive automatic
conversion would be too much overhead whe passing through FFI. The developer
should ensure the correct typings of the internal contained values
I know it may bring some convenience, but in this case I think we should not
do it.
Suggestion: Remove the conversion logic from this PR
##########
include/tvm/runtime/packed_func.h:
##########
@@ -547,29 +551,43 @@ class TVMPODValue_ {
// Allow automatic conversion from int to float
// This avoids errors when user pass in int from
// the frontend while the API expects a float.
- if (type_code_ == kDLInt) {
- return static_cast<double>(value_.v_int64);
+ if (auto opt = TryAsBool()) {
+ return opt.value();
+ } else if (auto opt = TryAsInt()) {
+ return opt.value();
+ } else if (auto opt = TryAsFloat()) {
+ return opt.value();
+ } else {
+ LOG(FATAL) << TVM_LOG_INCORRECT_TYPE_CODE(type_code_, kDLFloat);
}
- TVM_CHECK_TYPE_CODE(type_code_, kDLFloat);
- return value_.v_float64;
}
operator int64_t() const {
- TVM_CHECK_TYPE_CODE(type_code_, kDLInt);
- return value_.v_int64;
- }
- operator uint64_t() const {
- TVM_CHECK_TYPE_CODE(type_code_, kDLInt);
- return value_.v_int64;
+ if (auto opt = TryAsBool()) {
Review Comment:
We should go in the order of
TryAsInt, TryAsBool
Prioritize the most likely ones
##########
include/tvm/runtime/packed_func.h:
##########
@@ -627,13 +645,54 @@ class TVMPODValue_ {
template <typename TObjectRef>
inline TObjectRef AsObjectRef() const;
+ std::optional<int64_t> TryAsInt() const {
+ // Helper function to reduce duplication in the variable integer
+ // conversions. This is publicly exposed, as it can be useful in
+ // specializations of PackedFuncValueConverter.
+ if (auto opt = FromBoxed<int64_t>()) {
+ return opt.value();
+ } else if (type_code_ == kDLInt) {
+ return value_.v_int64;
+ } else {
+ return std::nullopt;
+ }
+ }
+
+ std::optional<double> TryAsFloat() const {
+ // Helper function to reduce duplication in the variable integer
+ // conversions. This is publicly exposed, as it can be useful in
+ // specializations of PackedFuncValueConverter.
+ if (auto opt = FromBoxed<double>()) {
Review Comment:
Calling convention note: it would be simpler to require that TVMArgValue and
TVMRetValue always NOT hold Boxed values. This would help to reduce the overall
cost of the codegen function handling.
That does mean when we assign Boxed Object into TVMArg and TVMRet, they
should be unboxed. I know that we can make an exception for bool for now until
we introduce a specific bool POD code.
##########
include/tvm/runtime/packed_func.h:
##########
@@ -2035,46 +2167,100 @@ inline TObjectRef TVMPODValue_::AsObjectRef() const {
ICHECK(!checked_type.defined()) << "Expected " <<
ObjectTypeChecker<TObjectRef>::TypeName()
<< ", but got " << checked_type.value();
return TObjectRef(GetObjectPtr<Object>(ptr));
- } else if (std::is_base_of<ContainerType, NDArray::ContainerType>::value &&
- type_code_ == kTVMNDArrayHandle) {
- // Casting to a base class that NDArray can sub-class
- ObjectPtr<Object> data =
-
NDArray::FFIDataFromHandle(static_cast<TVMArrayHandle>(value_.v_handle));
- return TObjectRef(data);
- } else if (std::is_base_of<ContainerType, Module::ContainerType>::value &&
- type_code_ == kTVMModuleHandle) {
- // Casting to a base class that Module can sub-class
- return
TObjectRef(GetObjectPtr<Object>(static_cast<Object*>(value_.v_handle)));
- } else if (std::is_base_of<ContainerType, PackedFunc::ContainerType>::value
&&
- type_code_ == kTVMPackedFuncHandle) {
- // Casting to a base class that PackedFunc can sub-class
- return
TObjectRef(GetObjectPtr<Object>(static_cast<Object*>(value_.v_handle)));
- } else {
- TVM_CHECK_TYPE_CODE(type_code_, kTVMObjectHandle);
- return TObjectRef(ObjectPtr<Object>(nullptr));
}
+
+ if constexpr (std::is_base_of_v<ContainerType, NDArray::ContainerType>) {
+ if (type_code_ == kTVMNDArrayHandle) {
+ // Casting to a base class that NDArray can sub-class
+ ObjectPtr<Object> data =
+
NDArray::FFIDataFromHandle(static_cast<TVMArrayHandle>(value_.v_handle));
+ return TObjectRef(data);
+ }
+ }
+
+ if constexpr (std::is_base_of_v<ContainerType, Module::ContainerType>) {
+ if (type_code_ == kTVMModuleHandle) {
+ // Casting to a base class that Module can sub-class
+ return
TObjectRef(GetObjectPtr<Object>(static_cast<Object*>(value_.v_handle)));
+ }
+ }
+
+ if constexpr (std::is_base_of_v<ContainerType, PackedFunc::ContainerType>) {
+ if (type_code_ == kTVMPackedFuncHandle) {
+ // Casting to a base class that PackedFunc can sub-class
+ return
TObjectRef(GetObjectPtr<Object>(static_cast<Object*>(value_.v_handle)));
+ }
+ }
+
+ if constexpr (std::is_base_of_v<TObjectRef, BoxInt>) {
+ if (type_code_ == kTVMArgInt) {
+ return BoxInt(value_.v_int64);
+ }
+ }
+
+ if constexpr (std::is_base_of_v<TObjectRef, BoxFloat>) {
+ if (type_code_ == kTVMArgFloat) {
+ return BoxFloat(value_.v_float64);
+ }
+ }
+
+ if constexpr (std::is_base_of_v<TObjectRef, String>) {
+ if (type_code_ == kTVMStr) {
+ return String(value_.v_str);
+ }
+ }
+
+ TVM_CHECK_TYPE_CODE(type_code_, kTVMObjectHandle);
+ return TObjectRef(ObjectPtr<Object>(nullptr));
}
template <typename TObjectRef, typename>
inline TVMRetValue& TVMRetValue::operator=(TObjectRef other) {
using ContainerType = typename TObjectRef::ContainerType;
const Object* ptr = other.get();
- if (ptr != nullptr) {
- if (std::is_base_of<NDArray::ContainerType, ContainerType>::value ||
- (std::is_base_of<ContainerType, NDArray::ContainerType>::value &&
- ptr->IsInstance<NDArray::ContainerType>())) {
+
+ if constexpr (std::is_base_of_v<ContainerType, NDArray::ContainerType> ||
+ std::is_base_of_v<NDArray::ContainerType, ContainerType>) {
+ if (ptr && (std::is_base_of_v<NDArray::ContainerType, ContainerType> ||
Review Comment:
nit: would be nice to keep the originals tructure of `if (ptr != nullptr)`,
they helps to keep the structure of the code, lift out a CSE and also makes
the diff more readable
--
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]
