[GitHub] [tvm] junrushao1994 commented on a change in pull request #10032: [Runtime][PackedFunc] Bring `PackedFunc` into TVM Object System

[GitBox <gi...@apache.org>]

junrushao1994 commented on a change in pull request #10032:
URL: https://github.com/apache/tvm/pull/10032#discussion_r790327773



##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -57,6 +57,73 @@ class TVMMovableArgValueWithContext_;
 class TVMRetValue;
 class TVMArgsSetter;
 
+/*!
+ * \brief Object container class that backs PackedFunc.
+ * \note Do not use this function directly, use PackedFunc.
+ */
+class PackedFuncObj : public Object {
+ public:
+  /*!
+   * \brief Call the function in packed format.
+   * \param args The arguments
+   * \param rv The return value.
+   */
+  inline void CallPacked(TVMArgs args, TVMRetValue* rv) const;

Review comment:
       nit
   
   ```suggestion
     inline TVM_ALWAYS_INLINE void CallPacked(TVMArgs args, TVMRetValue* rv) const;
   ```

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -65,36 +132,22 @@ class TVMArgsSetter;
  *  It is the unified function function type of TVM.
  *  It corresponds to TVMFunctionHandle in C runtime API.
  */
-class PackedFunc {
+class PackedFunc : public ObjectRef {
  public:
-  /*!
-   * \brief The internal std::function
-   * \param args The arguments to the function.
-   * \param rv The return value.
-   *
-   * \code
-   *   // Example code on how to implemented FType
-   *   void MyPackedFunc(TVMArgs args, TVMRetValue* rv) {
-   *     // automatically convert arguments to desired type.
-   *     int a0 = args[0];
-   *     float a1 = args[1];
-   *     ...
-   *     // automatically assign values to rv
-   *     std::string my_return_value = "x";
-   *     *rv = my_return_value;
-   *   }
-   * \endcode
-   */
-  using FType = std::function<void(TVMArgs args, TVMRetValue* rv)>;
-  /*! \brief default constructor */
-  PackedFunc() {}
   /*! \brief constructor from null */
-  PackedFunc(std::nullptr_t null) {}  // NOLINT(*)
+  PackedFunc(std::nullptr_t null): ObjectRef(nullptr) {}  // NOLINT(*)
   /*!
-   * \brief constructing a packed function from a std::function.
-   * \param body the internal container of packed function.
+   * \brief constructing a packed function from a type-erased callable type.
+   * \param data the internal container of packed function.
    */
-  explicit PackedFunc(FType body) : body_(body) {}
+  template <typename TCallable,
+            typename = std::enable_if_t<
+                std::is_convertible<TCallable, std::function<void(TVMArgs, TVMRetValue*)>>::value &&
+                !std::is_base_of<TCallable, PackedFunc>::value>>
+  explicit PackedFunc(TCallable data) {
+    using ObjType = PackedFuncSubObj<TCallable>;
+    data_ = make_object<ObjType>(std::forward<TCallable>(data));

Review comment:
       Would love to think more carefully if we need to implement copy/move constructor/operator as rule of five. Would you like to provide some pointers here? @tqchen 

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -57,6 +57,73 @@ class TVMMovableArgValueWithContext_;
 class TVMRetValue;
 class TVMArgsSetter;
 
+/*!
+ * \brief Object container class that backs PackedFunc.
+ * \note Do not use this function directly, use PackedFunc.
+ */
+class PackedFuncObj : public Object {
+ public:
+  /*!
+   * \brief Call the function in packed format.
+   * \param args The arguments
+   * \param rv The return value.
+   */
+  inline void CallPacked(TVMArgs args, TVMRetValue* rv) const;
+  
+  /*! \return Whether the packed function is nullptr */
+  bool operator==(std::nullptr_t null) const { return f_call_ == nullptr; }
+  /*! \return Whether the packed function is not nullptr */
+  bool operator!=(std::nullptr_t null) const { return f_call_ != nullptr; }
+
+  static constexpr const char* _type_key = "PackedFuncObj";

Review comment:
       Let's rename it according to our convention in StringObj, also please add a static type index: https://github.com/apache/tvm/blob/main/include/tvm/runtime/container/string.h#L92-L93.
   
   ```suggestion
     static constexpr const uint32_t _type_index = TypeIndex::kRuntimePackedFunc;
     static constexpr const char* _type_key = "runtime.PackedFunc";
   ```

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -57,6 +57,73 @@ class TVMMovableArgValueWithContext_;
 class TVMRetValue;
 class TVMArgsSetter;
 
+/*!
+ * \brief Object container class that backs PackedFunc.
+ * \note Do not use this function directly, use PackedFunc.
+ */
+class PackedFuncObj : public Object {
+ public:
+  /*!
+   * \brief Call the function in packed format.
+   * \param args The arguments
+   * \param rv The return value.
+   */
+  inline void CallPacked(TVMArgs args, TVMRetValue* rv) const;
+  
+  /*! \return Whether the packed function is nullptr */
+  bool operator==(std::nullptr_t null) const { return f_call_ == nullptr; }
+  /*! \return Whether the packed function is not nullptr */
+  bool operator!=(std::nullptr_t null) const { return f_call_ != nullptr; }
+
+  static constexpr const char* _type_key = "PackedFuncObj";
+  TVM_DECLARE_FINAL_OBJECT_INFO(PackedFuncObj, Object);
+
+ protected:
+  /*!
+  * \brief Internal struct for extracting the callable method from callable type.
+  */
+  template <class TPackedFuncSubObj>
+  struct Extractor {
+    /*! 
+     * \brief extracting the callable method from callable type. 
+     * \param obj The base packed function object class.
+     * \param args The arguments
+     * \param rv The return value.
+     */
+    static void Call(const PackedFuncObj* obj, TVMArgs args, TVMRetValue* rv);
+  };
+
+  /*! \brief The internal callable function type. */
+  using FCall = void(const PackedFuncObj*, TVMArgs, TVMRetValue*);

Review comment:
       nit: Rename to `FCallPacked`
   
   ```suggestion
     using FCallPacked = void(const PackedFuncObj*, TVMArgs, TVMRetValue*);
   ```

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1148,9 +1192,19 @@ inline TVMArgValue TVMArgs::operator[](int i) const {
 
 inline int TVMArgs::size() const { return num_args; }
 
-inline void PackedFunc::CallPacked(TVMArgs args, TVMRetValue* rv) const { body_(args, rv); }
+template <class TPackedFuncSubObj>
+void PackedFuncObj::Extractor<TPackedFuncSubObj>::Call(const PackedFuncObj* obj, TVMArgs args, TVMRetValue* rv) {
+  (static_cast<const TPackedFuncSubObj*>(obj))->callable_(args, rv);
+}
+
+inline void PackedFuncObj::CallPacked(TVMArgs args, TVMRetValue* rv) const {

Review comment:
       nit
   
   ```suggestion
   inline TVM_ALWAYS_INLINE void PackedFuncObj::CallPacked(TVMArgs args, TVMRetValue* rv) const {
   ```

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -57,6 +57,73 @@ class TVMMovableArgValueWithContext_;
 class TVMRetValue;
 class TVMArgsSetter;
 
+/*!
+ * \brief Object container class that backs PackedFunc.
+ * \note Do not use this function directly, use PackedFunc.
+ */
+class PackedFuncObj : public Object {
+ public:
+  /*!
+   * \brief Call the function in packed format.
+   * \param args The arguments
+   * \param rv The return value.
+   */
+  inline void CallPacked(TVMArgs args, TVMRetValue* rv) const;
+  
+  /*! \return Whether the packed function is nullptr */
+  bool operator==(std::nullptr_t null) const { return f_call_ == nullptr; }
+  /*! \return Whether the packed function is not nullptr */
+  bool operator!=(std::nullptr_t null) const { return f_call_ != nullptr; }
+
+  static constexpr const char* _type_key = "PackedFuncObj";
+  TVM_DECLARE_FINAL_OBJECT_INFO(PackedFuncObj, Object);
+
+ protected:
+  /*!
+  * \brief Internal struct for extracting the callable method from callable type.
+  */
+  template <class TPackedFuncSubObj>
+  struct Extractor {
+    /*! 
+     * \brief extracting the callable method from callable type. 
+     * \param obj The base packed function object class.
+     * \param args The arguments
+     * \param rv The return value.
+     */
+    static void Call(const PackedFuncObj* obj, TVMArgs args, TVMRetValue* rv);
+  };
+
+  /*! \brief The internal callable function type. */
+  using FCall = void(const PackedFuncObj*, TVMArgs, TVMRetValue*);
+  
+  /*!
+   * \brief Constructing a packed function object from a function pointer.
+   * \param f_call The function pointer used to call the packed function.
+   */
+  explicit PackedFuncObj(FCall* f_call) : f_call_(f_call) {}
+
+  /*! \brief Internal callable function pointer used to call the packed function. */
+  FCall* f_call_;

Review comment:
       nit
   
   ```suggestion
     FCallPacked* f_call_packed_;
   ```

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -1148,9 +1192,19 @@ inline TVMArgValue TVMArgs::operator[](int i) const {
 
 inline int TVMArgs::size() const { return num_args; }
 
-inline void PackedFunc::CallPacked(TVMArgs args, TVMRetValue* rv) const { body_(args, rv); }
+template <class TPackedFuncSubObj>
+void PackedFuncObj::Extractor<TPackedFuncSubObj>::Call(const PackedFuncObj* obj, TVMArgs args, TVMRetValue* rv) {
+  (static_cast<const TPackedFuncSubObj*>(obj))->callable_(args, rv);
+}
+
+inline void PackedFuncObj::CallPacked(TVMArgs args, TVMRetValue* rv) const {
+  (*f_call_)(this, args, rv); 
+}
+
 
-inline PackedFunc::FType PackedFunc::body() const { return body_; }
+inline void PackedFunc::CallPacked(TVMArgs args, TVMRetValue* rv) const {

Review comment:
       ```suggestion
   inline TVM_ALWAYS_INLINE void PackedFunc::CallPacked(TVMArgs args, TVMRetValue* rv) const {
   ```

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -117,16 +170,12 @@ class PackedFunc {
    * \param rv The return value.
    */
   inline void CallPacked(TVMArgs args, TVMRetValue* rv) const;
-  /*! \return the internal body function */
-  inline FType body() const;
   /*! \return Whether the packed function is nullptr */
-  bool operator==(std::nullptr_t null) const { return body_ == nullptr; }
+  bool operator==(std::nullptr_t null) const { return data_ == nullptr; }
   /*! \return Whether the packed function is not nullptr */
-  bool operator!=(std::nullptr_t null) const { return body_ != nullptr; }
+  bool operator!=(std::nullptr_t null) const { return data_ != nullptr; }

Review comment:
       Let's consider more carefully about the definition of nullability of PackedFunc. There are two cases:
   
   - C1. `PackedFunc::data_` is null
   - C2. `PackedFunc::data_::f_call_packed_` is null
   
   How do we formally define if a PackedFunc is null then?

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -117,16 +170,12 @@ class PackedFunc {
    * \param rv The return value.
    */
   inline void CallPacked(TVMArgs args, TVMRetValue* rv) const;

Review comment:
       nit
   
   ```suggestion
     inline TVM_ALWAYS_INLINE void CallPacked(TVMArgs args, TVMRetValue* rv) const;
   ```

##########
File path: include/tvm/runtime/packed_func.h
##########
@@ -65,36 +132,22 @@ class TVMArgsSetter;
  *  It is the unified function function type of TVM.
  *  It corresponds to TVMFunctionHandle in C runtime API.
  */
-class PackedFunc {
+class PackedFunc : public ObjectRef {
  public:
-  /*!
-   * \brief The internal std::function
-   * \param args The arguments to the function.
-   * \param rv The return value.
-   *
-   * \code
-   *   // Example code on how to implemented FType
-   *   void MyPackedFunc(TVMArgs args, TVMRetValue* rv) {
-   *     // automatically convert arguments to desired type.
-   *     int a0 = args[0];
-   *     float a1 = args[1];
-   *     ...
-   *     // automatically assign values to rv
-   *     std::string my_return_value = "x";
-   *     *rv = my_return_value;
-   *   }
-   * \endcode
-   */
-  using FType = std::function<void(TVMArgs args, TVMRetValue* rv)>;
-  /*! \brief default constructor */
-  PackedFunc() {}
   /*! \brief constructor from null */
-  PackedFunc(std::nullptr_t null) {}  // NOLINT(*)
+  PackedFunc(std::nullptr_t null): ObjectRef(nullptr) {}  // NOLINT(*)
   /*!
-   * \brief constructing a packed function from a std::function.
-   * \param body the internal container of packed function.
+   * \brief constructing a packed function from a type-erased callable type.

Review comment:
       ```suggestion
      * \brief constructing a packed function from a callable type whose signature is consistent with `PackedFunc`
   ```




-- 
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: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org