You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/12/22 23:24:49 UTC
[GitHub] [tvm] tkonolige opened a new pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
tkonolige opened a new pull request #7152:
URL: https://github.com/apache/tvm/pull/7152
This PR is an alternate to #7108, that captures the name of a TypedPackedFunc into a lambda instead of adding it as a field to the class. Right now, naming a TypedPackedFunc is optional, but I suggest we make it mandatory. I've already converted some of the places in the codebase to add a name (in this PR).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552918919
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -442,13 +447,17 @@ class TVMPODValue_ {
protected:
friend class TVMArgsSetter;
friend class TVMRetValue;
+ friend class TVMMovableArgValue_;
TVMPODValue_() : type_code_(kTVMNullptr) {}
- TVMPODValue_(TVMValue value, int type_code) : value_(value), type_code_(type_code) {}
+ TVMPODValue_(TVMValue value, int type_code, const std::string& prefix_message = "")
+ : value_(value), type_code_(type_code), prefix_message_(prefix_message) {}
/*! \brief The value */
TVMValue value_;
/*! \brief the type code */
int type_code_;
+ /*! \brief Message to prefix any error message with */
+ std::string prefix_message_;
Review comment:
I understand what this code does, want to point out the overhead in the current approach is too large that we might want to think about alternatives :)
The efficiency and clean part of the packedfunc mechanism really matters here that we might not want to include the feature because of the eager construction overhead.
I am not suggesting the FFI catching is the only way, but I do think more thoughts need to put into the process, for example, to have the error message report the i-th argument when things went wrong, while catching will show the name of the function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r548741004
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -559,7 +569,18 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
private:
/*! \return The arg value repr of the value. */
- TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
+ TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_, prefix_message_); }
+ /*! \brief Construct a message to provide context to any conversion errors in TVMPODValue_. */
+ static std::string error_string(const std::string& name, int argnum) {
+ if (name.size() == 0) {
+ return "";
+ }
+ std::string s = name + ": ";
Review comment:
perhaps should add something here to indicate that `name` is a function?
`std::string s = std::string{"in calling function "} + s + ": ";`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r556742042
##########
File path: include/tvm/ir/transform.h
##########
@@ -410,9 +410,30 @@ class Sequential : public Pass {
*
* \return The created module pass.
*/
-TVM_DLL Pass
-CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
- int opt_level, String name, Array<runtime::String> required);
+Pass CreateModulePass_(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
+ int opt_level, String name, Array<runtime::String> required);
+
+/*
+ * \brief Create a module pass.
+ *
+ * \param pass_func The function that contains the optimization.
+ * \param opt_level The optimization level of the module pass.
Review comment:
It would be great to leave these in a separate PR. Given the particular change will affect all PackedFunc calls. A smaller set of changes would be easier for others to review and take a look at
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#issuecomment-767892174
@junrushao1994 please take another look and manage the PR.
@tkonolige Thanks for keep improving the code through the reviewing process.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#issuecomment-764964882
@tqchen @areusch @junrushao1994 I think this PR is in a good state to merge. Could you review again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552886650
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -442,13 +447,17 @@ class TVMPODValue_ {
protected:
friend class TVMArgsSetter;
friend class TVMRetValue;
+ friend class TVMMovableArgValue_;
TVMPODValue_() : type_code_(kTVMNullptr) {}
- TVMPODValue_(TVMValue value, int type_code) : value_(value), type_code_(type_code) {}
+ TVMPODValue_(TVMValue value, int type_code, const std::string& prefix_message = "")
+ : value_(value), type_code_(type_code), prefix_message_(prefix_message) {}
/*! \brief The value */
TVMValue value_;
/*! \brief the type code */
int type_code_;
+ /*! \brief Message to prefix any error message with */
+ std::string prefix_message_;
Review comment:
> Instead, it might be good to think about alternatives. E.g. trying to be able to catch the errors in FFI functions and add name to the trace.
This code gives error messages when the arguments cannot be converted correctly. It prints which argument does not match. I don't see how catching errors at the FFI can tell us which argument was incorrect.
The only other approach I can think of is manually converting each argument before we call the function. This would be a much larger change and would probably involve some template magic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r557616275
##########
File path: include/tvm/ir/transform.h
##########
@@ -410,9 +410,30 @@ class Sequential : public Pass {
*
* \return The created module pass.
*/
-TVM_DLL Pass
-CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
- int opt_level, String name, Array<runtime::String> required);
+Pass CreateModulePass_(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
+ int opt_level, String name, Array<runtime::String> required);
+
+/*
+ * \brief Create a module pass.
+ *
+ * \param pass_func The function that contains the optimization.
+ * \param opt_level The optimization level of the module pass.
Review comment:
I've reverted these changes and will push them in a subsequent PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552916021
##########
File path: include/tvm/ir/transform.h
##########
@@ -414,6 +414,65 @@ TVM_DLL Pass
CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
int opt_level, String name, Array<runtime::String> required);
+class ModulePassNode : public PassNode {
Review comment:
To clarify.
- Have a function (not templated) `CreateModulePass_` that takes in `TypedPackedFunc`
- Create a templated function `CreateModulePass` that constructs the function(with name) and call into `CreateModulePass_`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552797360
##########
File path: include/tvm/ir/transform.h
##########
@@ -414,6 +414,65 @@ TVM_DLL Pass
CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
int opt_level, String name, Array<runtime::String> required);
+class ModulePassNode : public PassNode {
Review comment:
It is needed for `CreateModulePass` (https://github.com/apache/tvm/pull/7152/files#diff-99b9cb940bf19f00bb0d5956a156574f1f1940b026bff03fc0cb5f78c71d9faeR471). The new version of `CreateModulePass` needs to be templated so it accepts lambdas (and other functions) before they have been converted to `TypedPackedFunc`s. That way we can correctly set the name.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 merged pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #7152:
URL: https://github.com/apache/tvm/pull/7152
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562266666
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1377,7 +1449,7 @@ inline TObjectRef TVMPODValue_::AsObjectRef() const {
using ContainerType = typename TObjectRef::ContainerType;
if (type_code_ == kTVMNullptr) {
- ICHECK(TObjectRef::_type_is_nullable)
+ CHECK(TObjectRef::_type_is_nullable)
Review comment:
shouldn't this be static_assert?
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1297,14 +1353,30 @@ TypedPackedFunc<R(Args...)>::TypedPackedFunc(const TVMArgValue& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
-TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValue_&& value)
+TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValueWithContext_&& value)
: packed_(value.operator PackedFunc()) {}
+template <typename R, typename... Args>
+template <typename FType>
+inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda, std::string name) {
+ packed_ = PackedFunc([flambda, name](const TVMArgs& args, TVMRetValue* rv) {
+ if (args.size() != sizeof...(Args)) {
+ LOG(FATAL) << "Function " << name << " expects " << sizeof...(Args) << " arguments, but "
Review comment:
maybe standardize the previous error message formatting with this one too
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1297,14 +1353,30 @@ TypedPackedFunc<R(Args...)>::TypedPackedFunc(const TVMArgValue& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
-TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValue_&& value)
+TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValueWithContext_&& value)
: packed_(value.operator PackedFunc()) {}
+template <typename R, typename... Args>
+template <typename FType>
+inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda, std::string name) {
+ packed_ = PackedFunc([flambda, name](const TVMArgs& args, TVMRetValue* rv) {
+ if (args.size() != sizeof...(Args)) {
+ LOG(FATAL) << "Function " << name << " expects " << sizeof...(Args) << " arguments, but "
+ << args.size() << " were provided.";
+ }
+ detail::unpack_call<R, sizeof...(Args)>(&name, flambda, args, rv);
+ });
+}
+
template <typename R, typename... Args>
template <typename FType>
inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda) {
packed_ = PackedFunc([flambda](const TVMArgs& args, TVMRetValue* rv) {
- detail::unpack_call<R, sizeof...(Args)>(flambda, args, rv);
+ if (args.size() != sizeof...(Args)) {
+ LOG(FATAL) << "Function <anonymous> expects " << sizeof...(Args) << " arguments, but "
Review comment:
and here as well
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1240,16 +1293,19 @@ struct unpack_call_dispatcher<R, 0, index, F> {
template <int index, typename F>
struct unpack_call_dispatcher<void, 0, index, F> {
template <typename... Args>
- TVM_ALWAYS_INLINE static void run(const F& f, const TVMArgs& args_pack, TVMRetValue* rv,
+ TVM_ALWAYS_INLINE static void run(const std::string* optional_name, const F& f,
+ const TVMArgs& args_pack, TVMRetValue* rv,
Args&&... unpacked_args) {
f(std::forward<Args>(unpacked_args)...);
}
};
template <typename R, int nargs, typename F>
-TVM_ALWAYS_INLINE void unpack_call(const F& f, const TVMArgs& args, TVMRetValue* rv) {
- ICHECK_EQ(nargs, args.size()) << "Expect " << nargs << " arguments but get " << args.size();
- unpack_call_dispatcher<R, nargs, 0, F>::run(f, args, rv);
+TVM_ALWAYS_INLINE void unpack_call(const std::string* optional_name, const F& f,
+ const TVMArgs& args, TVMRetValue* rv) {
+ CHECK_EQ(nargs, args.size()) << (optional_name == nullptr ? "<anonymous>" : *optional_name)
Review comment:
maybe clarify what is the anonymous thing? i.e. `<anonymous function>` or like:
`CHECK_EQ(...) << "function " << (optional_name ...) << ": expected " << nargs << " arguments` ...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562270981
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1377,7 +1449,7 @@ inline TObjectRef TVMPODValue_::AsObjectRef() const {
using ContainerType = typename TObjectRef::ContainerType;
if (type_code_ == kTVMNullptr) {
- ICHECK(TObjectRef::_type_is_nullable)
+ CHECK(TObjectRef::_type_is_nullable)
Review comment:
this should actually be a runtime check, note that constant folding will actually remove this completely
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -229,13 +229,13 @@ class TypedPackedFunc<R(Args...)> {
* \endcode
*
* \param typed_lambda typed lambda function.
+ * \param name the name of the lambda function.
* \tparam FLambda the type of the lambda function.
*/
- template <typename FLambda, typename = typename std::enable_if<
- std::is_convertible<FLambda,
- std::function<R(Args...)>>::value>::type>
- TypedPackedFunc(const FLambda& typed_lambda) { // NOLINT(*)
- this->AssignTypedLambda(typed_lambda);
+ template <typename FLambda, typename = typename std::enable_if<std::is_convertible<
+ FLambda, std::function<R(Args...)>>::value>::type>
+ TypedPackedFunc(const FLambda& typed_lambda, std::string name = "<anonymous>") { // NOLINT(*)
Review comment:
We need two constructors, one that has the string argument and another that does not.
The one that is anonymous should not capture any string content or construct a string
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552924017
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -442,13 +447,17 @@ class TVMPODValue_ {
protected:
friend class TVMArgsSetter;
friend class TVMRetValue;
+ friend class TVMMovableArgValue_;
TVMPODValue_() : type_code_(kTVMNullptr) {}
- TVMPODValue_(TVMValue value, int type_code) : value_(value), type_code_(type_code) {}
+ TVMPODValue_(TVMValue value, int type_code, const std::string& prefix_message = "")
+ : value_(value), type_code_(type_code), prefix_message_(prefix_message) {}
/*! \brief The value */
TVMValue value_;
/*! \brief the type code */
int type_code_;
+ /*! \brief Message to prefix any error message with */
+ std::string prefix_message_;
Review comment:
Happy to chat more, i think there could be a better approach here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#issuecomment-768395015
Yeah, a type mismatch macro could be useful.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r548759022
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -559,7 +569,18 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
private:
/*! \return The arg value repr of the value. */
- TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
+ TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_, prefix_message_); }
+ /*! \brief Construct a message to provide context to any conversion errors in TVMPODValue_. */
+ static std::string error_string(const std::string& name, int argnum) {
Review comment:
Please do not write a static function in c++ header...Use inline to make sure only one copy of this function exists after linking.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#issuecomment-755459243
Taking a closer look, i think we need to put more thoughts into it to make sure that the addition of name is optional, and we can get close to zero overhead in building these functions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562750127
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1240,16 +1293,19 @@ struct unpack_call_dispatcher<R, 0, index, F> {
template <int index, typename F>
struct unpack_call_dispatcher<void, 0, index, F> {
template <typename... Args>
- TVM_ALWAYS_INLINE static void run(const F& f, const TVMArgs& args_pack, TVMRetValue* rv,
+ TVM_ALWAYS_INLINE static void run(const std::string* optional_name, const F& f,
+ const TVMArgs& args_pack, TVMRetValue* rv,
Args&&... unpacked_args) {
f(std::forward<Args>(unpacked_args)...);
}
};
template <typename R, int nargs, typename F>
-TVM_ALWAYS_INLINE void unpack_call(const F& f, const TVMArgs& args, TVMRetValue* rv) {
- ICHECK_EQ(nargs, args.size()) << "Expect " << nargs << " arguments but get " << args.size();
- unpack_call_dispatcher<R, nargs, 0, F>::run(f, args, rv);
+TVM_ALWAYS_INLINE void unpack_call(const std::string* optional_name, const F& f,
+ const TVMArgs& args, TVMRetValue* rv) {
+ CHECK_EQ(nargs, args.size()) << (optional_name == nullptr ? "<anonymous>" : *optional_name)
Review comment:
Good point Andrew. I've changed the messages so they are all "Function \<anonymous\> expects....".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562750692
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -229,13 +229,13 @@ class TypedPackedFunc<R(Args...)> {
* \endcode
*
* \param typed_lambda typed lambda function.
+ * \param name the name of the lambda function.
* \tparam FLambda the type of the lambda function.
*/
- template <typename FLambda, typename = typename std::enable_if<
- std::is_convertible<FLambda,
- std::function<R(Args...)>>::value>::type>
- TypedPackedFunc(const FLambda& typed_lambda) { // NOLINT(*)
- this->AssignTypedLambda(typed_lambda);
+ template <typename FLambda, typename = typename std::enable_if<std::is_convertible<
+ FLambda, std::function<R(Args...)>>::value>::type>
+ TypedPackedFunc(const FLambda& typed_lambda, std::string name = "<anonymous>") { // NOLINT(*)
Review comment:
Good catch. I totally forgot about that constructor.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562744494
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1377,7 +1449,7 @@ inline TObjectRef TVMPODValue_::AsObjectRef() const {
using ContainerType = typename TObjectRef::ContainerType;
if (type_code_ == kTVMNullptr) {
- ICHECK(TObjectRef::_type_is_nullable)
+ CHECK(TObjectRef::_type_is_nullable)
Review comment:
I think this is out of scope for this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562270981
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1377,7 +1449,7 @@ inline TObjectRef TVMPODValue_::AsObjectRef() const {
using ContainerType = typename TObjectRef::ContainerType;
if (type_code_ == kTVMNullptr) {
- ICHECK(TObjectRef::_type_is_nullable)
+ CHECK(TObjectRef::_type_is_nullable)
Review comment:
this should actually be a runtime check, note that constant folding will actually remove this completely
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#issuecomment-767910081
Will do tonight!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r556737215
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -562,6 +570,37 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
};
+/*!
+ * \brief Internal auxiliary struct for TypedPackedFunc to indicate a movable argument
+ * with additional context information for better error reporting.
+ *
+ * \sa MovableArgValue_
+ * \note For internal development purpose only.
+ */
+class TVMMovableArgValueWithContext_ {
+ public:
+ public:
+ TVMMovableArgValueWithContext_(TVMValue value, int type_code, int arg_index,
+ const std::string* name)
+ : value_(value, type_code), arg_index_(arg_index), name_(name) {}
+
+ template <typename T>
+ inline operator T() const {
Review comment:
when the function is inside a class body, iline is not needed
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -562,6 +570,37 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
};
+/*!
+ * \brief Internal auxiliary struct for TypedPackedFunc to indicate a movable argument
+ * with additional context information for better error reporting.
+ *
+ * \sa MovableArgValue_
+ * \note For internal development purpose only.
+ */
+class TVMMovableArgValueWithContext_ {
+ public:
+ public:
Review comment:
two public, document the arguments
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -562,6 +570,37 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
};
+/*!
+ * \brief Internal auxiliary struct for TypedPackedFunc to indicate a movable argument
+ * with additional context information for better error reporting.
+ *
+ * \sa MovableArgValue_
+ * \note For internal development purpose only.
+ */
+class TVMMovableArgValueWithContext_ {
+ public:
+ public:
+ TVMMovableArgValueWithContext_(TVMValue value, int type_code, int arg_index,
+ const std::string* name)
+ : value_(value, type_code), arg_index_(arg_index), name_(name) {}
+
+ template <typename T>
+ inline operator T() const {
+ try {
+ return value_;
+ } catch (dmlc::Error& e) {
+ LOG(FATAL) << "In function " << (name_ == nullptr ? "<anonymous>" : *name_)
+ << ": error while converting argument " << arg_index_ << ": " << e.what();
+ throw "";
+ }
+ }
+
+ private:
+ TVMMovableArgValue_ value_;
+ int arg_index_;
+ const std::string* name_;
Review comment:
rename to opt_name_ indicate this is optional
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1297,14 +1339,18 @@ TypedPackedFunc<R(Args...)>::TypedPackedFunc(const TVMArgValue& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
-TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValue_&& value)
+TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValueWithContext_&& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
template <typename FType>
-inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda) {
- packed_ = PackedFunc([flambda](const TVMArgs& args, TVMRetValue* rv) {
Review comment:
Let us keep an implementation when name is not provided, and no capturing is needed
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1297,14 +1339,18 @@ TypedPackedFunc<R(Args...)>::TypedPackedFunc(const TVMArgValue& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
-TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValue_&& value)
+TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValueWithContext_&& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
template <typename FType>
-inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda) {
- packed_ = PackedFunc([flambda](const TVMArgs& args, TVMRetValue* rv) {
- detail::unpack_call<R, sizeof...(Args)>(flambda, args, rv);
+inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda, std::string name) {
+ packed_ = PackedFunc([flambda, name](const TVMArgs& args, TVMRetValue* rv) {
+ if (args.size() != sizeof...(Args)) {
+ LOG(FATAL) << "Function " << (name.size() == 0 ? "<anonymous>" : name) << " expects "
Review comment:
in this particular name is provided, and we can always print a name
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -336,8 +337,11 @@ class TVMArgs {
inline const char* ArgTypeCode2Str(int type_code);
// macro to check type code.
+#define TVM_ICHECK_TYPE_CODE(CODE, T) \
+ ICHECK_EQ(CODE, T) << " expected " << ArgTypeCode2Str(T) << " but got " << ArgTypeCode2Str(CODE)
Review comment:
Given the macro is only used inside the packed_func, let us only keep one version. TVM_CHECK_TYPE_CODE should be fine for this case. (feel free to use ICHECK internally)
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -562,6 +570,37 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
};
+/*!
+ * \brief Internal auxiliary struct for TypedPackedFunc to indicate a movable argument
+ * with additional context information for better error reporting.
+ *
Review comment:
additional context information such as function name and argument index
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -526,7 +533,8 @@ class TVMArgValue : public TVMPODValue_ {
*/
class TVMMovableArgValue_ : public TVMPODValue_ {
public:
- TVMMovableArgValue_(TVMValue value, int type_code) : TVMPODValue_(value, type_code) {}
+ TVMMovableArgValue_(TVMValue value, int type_code, const std::string& name = "", int argnum = -1)
Review comment:
this change is no longer needed
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -562,6 +570,37 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
};
+/*!
+ * \brief Internal auxiliary struct for TypedPackedFunc to indicate a movable argument
+ * with additional context information for better error reporting.
+ *
+ * \sa MovableArgValue_
+ * \note For internal development purpose only.
+ */
+class TVMMovableArgValueWithContext_ {
+ public:
+ public:
+ TVMMovableArgValueWithContext_(TVMValue value, int type_code, int arg_index,
+ const std::string* name)
+ : value_(value, type_code), arg_index_(arg_index), name_(name) {}
+
+ template <typename T>
+ inline operator T() const {
+ try {
+ return value_;
+ } catch (dmlc::Error& e) {
+ LOG(FATAL) << "In function " << (name_ == nullptr ? "<anonymous>" : *name_)
+ << ": error while converting argument " << arg_index_ << ": " << e.what();
+ throw "";
Review comment:
add a comment about throw
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1213,21 +1252,22 @@ namespace detail {
template <typename R, int nleft, int index, typename F>
struct unpack_call_dispatcher {
template <typename... Args>
- TVM_ALWAYS_INLINE static void run(const F& f, const TVMArgs& args_pack, TVMRetValue* rv,
- Args&&... unpacked_args) {
+ TVM_ALWAYS_INLINE static void run(const std::string* name, const F& f, const TVMArgs& args_pack,
+ TVMRetValue* rv, Args&&... unpacked_args) {
// construct a movable argument value
// which allows potential move of argument to the input of F.
unpack_call_dispatcher<R, nleft - 1, index + 1, F>::run(
- f, args_pack, rv, std::forward<Args>(unpacked_args)...,
- TVMMovableArgValue_(args_pack.values[index], args_pack.type_codes[index]));
+ name, f, args_pack, rv, std::forward<Args>(unpacked_args)...,
+ TVMMovableArgValueWithContext_(args_pack.values[index], args_pack.type_codes[index], index,
+ name));
}
};
template <typename R, int index, typename F>
struct unpack_call_dispatcher<R, 0, index, F> {
template <typename... Args>
- TVM_ALWAYS_INLINE static void run(const F& f, const TVMArgs& args_pack, TVMRetValue* rv,
- Args&&... unpacked_args) {
+ TVM_ALWAYS_INLINE static void run(const std::string* name, const F& f, const TVMArgs& args_pack,
Review comment:
name -> optional_name
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r548758807
##########
File path: include/tvm/ir/transform.h
##########
@@ -414,6 +414,65 @@ TVM_DLL Pass
CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
int opt_level, String name, Array<runtime::String> required);
+class ModulePassNode : public PassNode {
Review comment:
I wonder why we need to move those pass definitions to a different place..Is this related to the error messages?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552855040
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -442,13 +447,17 @@ class TVMPODValue_ {
protected:
friend class TVMArgsSetter;
friend class TVMRetValue;
+ friend class TVMMovableArgValue_;
TVMPODValue_() : type_code_(kTVMNullptr) {}
- TVMPODValue_(TVMValue value, int type_code) : value_(value), type_code_(type_code) {}
+ TVMPODValue_(TVMValue value, int type_code, const std::string& prefix_message = "")
+ : value_(value), type_code_(type_code), prefix_message_(prefix_message) {}
/*! \brief The value */
TVMValue value_;
/*! \brief the type code */
int type_code_;
+ /*! \brief Message to prefix any error message with */
+ std::string prefix_message_;
Review comment:
Adding String to each of the argument is a big overhead. We certainly don't want to construct such an error message for each function call even if no error is presented.
Instead, it might be good to think about alternatives. E.g. trying to be able to catch the errors in FFI functions and add name to the trace.
##########
File path: include/tvm/ir/transform.h
##########
@@ -414,6 +414,65 @@ TVM_DLL Pass
CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
int opt_level, String name, Array<runtime::String> required);
+class ModulePassNode : public PassNode {
Review comment:
Additionally, this could be something that we can add back once the packed func land
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562750127
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1240,16 +1293,19 @@ struct unpack_call_dispatcher<R, 0, index, F> {
template <int index, typename F>
struct unpack_call_dispatcher<void, 0, index, F> {
template <typename... Args>
- TVM_ALWAYS_INLINE static void run(const F& f, const TVMArgs& args_pack, TVMRetValue* rv,
+ TVM_ALWAYS_INLINE static void run(const std::string* optional_name, const F& f,
+ const TVMArgs& args_pack, TVMRetValue* rv,
Args&&... unpacked_args) {
f(std::forward<Args>(unpacked_args)...);
}
};
template <typename R, int nargs, typename F>
-TVM_ALWAYS_INLINE void unpack_call(const F& f, const TVMArgs& args, TVMRetValue* rv) {
- ICHECK_EQ(nargs, args.size()) << "Expect " << nargs << " arguments but get " << args.size();
- unpack_call_dispatcher<R, nargs, 0, F>::run(f, args, rv);
+TVM_ALWAYS_INLINE void unpack_call(const std::string* optional_name, const F& f,
+ const TVMArgs& args, TVMRetValue* rv) {
+ CHECK_EQ(nargs, args.size()) << (optional_name == nullptr ? "<anonymous>" : *optional_name)
Review comment:
Good point Andrew. I've changed the messages so they are all "Function <anonymous> expects....".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r565075157
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -562,6 +597,44 @@ class TVMMovableArgValue_ : public TVMPODValue_ {
TVMArgValue AsArgValue() const { return TVMArgValue(value_, type_code_); }
};
+/*!
+ * \brief Internal auxiliary struct for TypedPackedFunc to indicate a movable argument with
+ * additional context information (function name and argument index) for better error reporting.
+ *
+ * \sa MovableArgValue_
+ * \note For internal development purpose only.
+ */
+class TVMMovableArgValueWithContext_ {
+ public:
+ /*!
+ * \brief move constructor from another return value.
+ * \param value The other return value.
+ * \param type_code The code associated with the type of the value.
+ * \param arg_index In a function call, this argument is at index arg_index (0-indexed).
+ * \param optional_name Name of the function being called. Can be nullptr if the function is not
+ * named.
+ */
+ TVMMovableArgValueWithContext_(TVMValue value, int type_code, int arg_index,
+ const std::string* optional_name)
+ : value_(value, type_code), arg_index_(arg_index), optional_name_(optional_name) {}
+
+ template <typename T>
+ operator T() const {
+ try {
+ return value_; // implicit conversion happens here
+ } catch (dmlc::Error& e) {
+ LOG(FATAL) << "In function " << (optional_name_ == nullptr ? "<anonymous>" : *optional_name_)
+ << ": error while converting argument " << arg_index_ << ": " << e.what();
+ throw ""; // never reached, LOG(FATAL) throws, but this silences a warning.
Review comment:
Looks like we don't have to throw anything haha
```suggestion
throw; // never reached, LOG(FATAL) throws, but this silences a warning.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#issuecomment-764964882
@tqchen @areusch @junrushao1994 I think this PR is in a good state to merge. Could you review again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552872362
##########
File path: include/tvm/ir/transform.h
##########
@@ -414,6 +414,65 @@ TVM_DLL Pass
CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
int opt_level, String name, Array<runtime::String> required);
+class ModulePassNode : public PassNode {
Review comment:
We cannot take a `TypedPackedFunc` in `CreateModulePass` because the name needs to be provided to the constructor of `TypedPackedFunc`. We cannot set it after the fact.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562266666
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1377,7 +1449,7 @@ inline TObjectRef TVMPODValue_::AsObjectRef() const {
using ContainerType = typename TObjectRef::ContainerType;
if (type_code_ == kTVMNullptr) {
- ICHECK(TObjectRef::_type_is_nullable)
+ CHECK(TObjectRef::_type_is_nullable)
Review comment:
shouldn't this be static_assert?
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1297,14 +1353,30 @@ TypedPackedFunc<R(Args...)>::TypedPackedFunc(const TVMArgValue& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
-TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValue_&& value)
+TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValueWithContext_&& value)
: packed_(value.operator PackedFunc()) {}
+template <typename R, typename... Args>
+template <typename FType>
+inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda, std::string name) {
+ packed_ = PackedFunc([flambda, name](const TVMArgs& args, TVMRetValue* rv) {
+ if (args.size() != sizeof...(Args)) {
+ LOG(FATAL) << "Function " << name << " expects " << sizeof...(Args) << " arguments, but "
Review comment:
maybe standardize the previous error message formatting with this one too
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1297,14 +1353,30 @@ TypedPackedFunc<R(Args...)>::TypedPackedFunc(const TVMArgValue& value)
: packed_(value.operator PackedFunc()) {}
template <typename R, typename... Args>
-TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValue_&& value)
+TypedPackedFunc<R(Args...)>::TypedPackedFunc(TVMMovableArgValueWithContext_&& value)
: packed_(value.operator PackedFunc()) {}
+template <typename R, typename... Args>
+template <typename FType>
+inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda, std::string name) {
+ packed_ = PackedFunc([flambda, name](const TVMArgs& args, TVMRetValue* rv) {
+ if (args.size() != sizeof...(Args)) {
+ LOG(FATAL) << "Function " << name << " expects " << sizeof...(Args) << " arguments, but "
+ << args.size() << " were provided.";
+ }
+ detail::unpack_call<R, sizeof...(Args)>(&name, flambda, args, rv);
+ });
+}
+
template <typename R, typename... Args>
template <typename FType>
inline void TypedPackedFunc<R(Args...)>::AssignTypedLambda(FType flambda) {
packed_ = PackedFunc([flambda](const TVMArgs& args, TVMRetValue* rv) {
- detail::unpack_call<R, sizeof...(Args)>(flambda, args, rv);
+ if (args.size() != sizeof...(Args)) {
+ LOG(FATAL) << "Function <anonymous> expects " << sizeof...(Args) << " arguments, but "
Review comment:
and here as well
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1240,16 +1293,19 @@ struct unpack_call_dispatcher<R, 0, index, F> {
template <int index, typename F>
struct unpack_call_dispatcher<void, 0, index, F> {
template <typename... Args>
- TVM_ALWAYS_INLINE static void run(const F& f, const TVMArgs& args_pack, TVMRetValue* rv,
+ TVM_ALWAYS_INLINE static void run(const std::string* optional_name, const F& f,
+ const TVMArgs& args_pack, TVMRetValue* rv,
Args&&... unpacked_args) {
f(std::forward<Args>(unpacked_args)...);
}
};
template <typename R, int nargs, typename F>
-TVM_ALWAYS_INLINE void unpack_call(const F& f, const TVMArgs& args, TVMRetValue* rv) {
- ICHECK_EQ(nargs, args.size()) << "Expect " << nargs << " arguments but get " << args.size();
- unpack_call_dispatcher<R, nargs, 0, F>::run(f, args, rv);
+TVM_ALWAYS_INLINE void unpack_call(const std::string* optional_name, const F& f,
+ const TVMArgs& args, TVMRetValue* rv) {
+ CHECK_EQ(nargs, args.size()) << (optional_name == nullptr ? "<anonymous>" : *optional_name)
Review comment:
maybe clarify what is the anonymous thing? i.e. `<anonymous function>` or like:
`CHECK_EQ(...) << "function " << (optional_name ...) << ": expected " << nargs << " arguments` ...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r562281721
##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -229,13 +229,13 @@ class TypedPackedFunc<R(Args...)> {
* \endcode
*
* \param typed_lambda typed lambda function.
+ * \param name the name of the lambda function.
* \tparam FLambda the type of the lambda function.
*/
- template <typename FLambda, typename = typename std::enable_if<
- std::is_convertible<FLambda,
- std::function<R(Args...)>>::value>::type>
- TypedPackedFunc(const FLambda& typed_lambda) { // NOLINT(*)
- this->AssignTypedLambda(typed_lambda);
+ template <typename FLambda, typename = typename std::enable_if<std::is_convertible<
+ FLambda, std::function<R(Args...)>>::value>::type>
+ TypedPackedFunc(const FLambda& typed_lambda, std::string name = "<anonymous>") { // NOLINT(*)
Review comment:
We need two constructors, one that has the string argument and another that does not.
The one that is anonymous should not capture any string content or construct a string
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#discussion_r552850438
##########
File path: include/tvm/ir/transform.h
##########
@@ -414,6 +414,65 @@ TVM_DLL Pass
CreateModulePass(const runtime::TypedPackedFunc<IRModule(IRModule, PassContext)>& pass_func,
int opt_level, String name, Array<runtime::String> required);
+class ModulePassNode : public PassNode {
Review comment:
I do think it is possible to keep things as they are(not moving to the header). Since the original rationale is to bring benefit of isolation. Ideally we don't want to cut one part to make things easier in another case.
To resolve this, one way is to keep the API in their original form, while creating a separate function CreateModulePass_ that takes the `runtime::TypedPackedFunc` which constructs during the templated version.
One possible way is to create an overloaded function of CreateModulePass_ that accepts the runtime::TypedPackedFunc which get calls into it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7152: [RUNTIME] Improve error messages for TypedPackedFunc
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7152:
URL: https://github.com/apache/tvm/pull/7152#issuecomment-769407463
Thank you @tkonolige @tqchen @areusch! It is now merged!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org