[GitHub] [tvm] masahi commented on a diff in pull request #15137: [Relay] Introduce arguments limit to FuseOps pass

["masahi (via GitHub)" <gi...@apache.org>]

masahi commented on code in PR #15137:
URL: https://github.com/apache/tvm/pull/15137#discussion_r1258806135


##########
include/tvm/relay/transform.h:
##########
@@ -120,9 +120,12 @@ TVM_DLL Pass FoldConstant(bool fold_qnn = false);
 /*!
  * \brief Split function with huge number of arguments to smaller pieces.
  *
+ * \param max_function_args Maximum number of function arguments. If it is 0 then SplitArgs won't
+ *                          split funciton.

Review Comment:
   typo



##########
src/relay/analysis/graph_partitioner.cc:
##########
@@ -238,6 +320,8 @@ void GraphPartitioner::InitGroups(const IndexedForwardGraph& graph) {
 void GraphPartitioner::RunFuse(const IndexedForwardGraph& graph,    //
                                const DominatorTree& post_dom_tree,  //
                                int phase) {
+  IndexedForwardGraph::Node* prev_node = nullptr;

Review Comment:
   Describe what is meant by "previous". It is only used for counting args but the name is too generic. 



##########
src/relay/analysis/graph_partitioner.h:
##########
@@ -247,6 +255,9 @@ class GraphPartitioner {
   void CommitFuse(IndexedForwardGraph::Node* src, IndexedForwardGraph::Node* sink);
 
   size_t CountNodesUptoSink_(IndexedForwardGraph::Node* src, IndexedForwardGraph::Node* sink);
+  size_t CountAdditionalArgs_(const TensorTypeNode* ttype, bool with_strides = true);

Review Comment:
   Describe what "Additional" means



##########
src/relay/analysis/graph_partitioner.h:
##########
@@ -247,6 +255,9 @@ class GraphPartitioner {
   void CommitFuse(IndexedForwardGraph::Node* src, IndexedForwardGraph::Node* sink);
 
   size_t CountNodesUptoSink_(IndexedForwardGraph::Node* src, IndexedForwardGraph::Node* sink);
+  size_t CountAdditionalArgs_(const TensorTypeNode* ttype, bool with_strides = true);
+  size_t CountArgs_(const tvm::Object* child, const tvm::Object* till_node);
+  size_t CountArgsLimit_(const IndexedForwardGraph::Node* child);

Review Comment:
   Describe what this function is for, even though there is already `max_function_args_`.



##########
src/relay/analysis/graph_partitioner.h:
##########
@@ -205,10 +209,14 @@ class GraphPartitioner {
   int opt_level_;
   /*! \brief The maximum number of operations in one fused function */
   size_t max_fuse_depth_;
+  /*! \brief The maximum number of arguments in one fused function */
+  size_t max_function_args_;
   /*! \brief The internal groups. */
   std::vector<Group*> groups_;
   /*! \brief internal field used for deduplication */
   std::unordered_set<IndexedForwardGraph::Node*> visited_;
+  /*! \brief internal field used for hashing arguments number for a node */
+  std::unordered_map<const tvm::Object*, size_t> argsMap_;

Review Comment:
   args_map_
   
   "arguments number" is a weird phrase 



##########
src/relay/analysis/graph_partitioner.h:
##########
@@ -205,10 +209,14 @@ class GraphPartitioner {
   int opt_level_;
   /*! \brief The maximum number of operations in one fused function */
   size_t max_fuse_depth_;
+  /*! \brief The maximum number of arguments in one fused function */
+  size_t max_function_args_;
   /*! \brief The internal groups. */
   std::vector<Group*> groups_;
   /*! \brief internal field used for deduplication */
   std::unordered_set<IndexedForwardGraph::Node*> visited_;
+  /*! \brief internal field used for hashing arguments number for a node */
+  std::unordered_map<const tvm::Object*, size_t> argsMap_;

Review Comment:
   Also the name `args_map_` doesn't tell what it is for. 



-- 
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