[GitHub] [tvm] mbaret commented on a diff in pull request #11481: [Relay] IndexedGraph improvements in preparation for Collage

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

mbaret commented on code in PR #11481:
URL: https://github.com/apache/tvm/pull/11481#discussion_r890137106


##########
include/tvm/relay/transform.h:
##########
@@ -281,6 +281,11 @@ TVM_DLL Pass InferType();
  */
 TVM_DLL Type InferTypeLocal(const Expr& expr);
 
+/*!
+ * \brief Infer the types of all sub-expression of expr.
+ */
+TVM_DLL Expr InferTypeExpr(const Expr& expr);
+

Review Comment:
   I can't see any usage of this in the PR - is it required?



##########
python/tvm/relay/transform/transform.py:
##########
@@ -802,24 +802,6 @@ def Inline():
     return _ffi_api.Inline()
 
 
-def InlineComposites(target):
-    """Perform inlining on the given Relay IR module. The functions originate
-    from the MergeComposite pass based on an input pattern table will fold back
-    to main. Currently, this is used for the TRT BYOC which expects a single
-    primitive function to operate on.
-
-    Parameters
-    ----------
-    target: str
-        The byoc target for which ops need to fold back to primitive function.
-    Returns
-    -------
-    ret: tvm.transform.Pass
-        The registered pass that performs inlining for a Relay IR module.
-    """
-    return _ffi_api.InlineComposites(target)
-
-

Review Comment:
   I think changes to this effect really should be separate straightforward PRs. We can get them merged quickly and straightforwardly and it'll improve the clarity of the history.



##########
tests/python/relay/test_dataflow_pattern.py:
##########
@@ -601,6 +600,64 @@ def test_match_fake_diamond():
     assert not diamond.match(out)
 
 
+def test_at_most_one_parent():

Review Comment:
   Just to confirm, do these two new tests fail without your changes/fixes to IndexedGraph?



##########
src/relay/ir/indexed_graph.h:
##########
@@ -19,7 +19,11 @@
 
 /*!
  * \file src/relay/ir/indexed_graph.h
- * \brief A pattern matcher for matching dataflow properties.
+ * \brief A graph representation of the dataflow in a Relay expression or Relay dataflow
+ * pattern. Nodes in this graph capture generic dataflow inputs, outputs, dominators and control
+ * flow scopes.
+ *
+ * TODO(mbs): Rename to 'DataflowGraph'.

Review Comment:
   I think either we should do the renaming, or remove the TODO.



##########
tests/python/contrib/test_ethosu/test_codegen.py:
##########
@@ -1061,7 +1061,4 @@ def fully_connected(x):
 
 
 if __name__ == "__main__":
-    import sys
-    import pytest
-
-    sys.exit(pytest.main([__file__] + sys.argv[1:]))
+    tvm.testing.main()

Review Comment:
   Seems like a random change?



##########
src/relay/op/dyn/tensor/transform.cc:
##########
@@ -258,6 +258,7 @@ RELAY_REGISTER_OP("dyn.broadcast_to")
     .describe(R"code(Broadcast the first input to match the shape argument.
 )code" TVM_ADD_FILELINE)
     .set_num_inputs(2)
+    .set_attrs_type<InitOpAttrs>()

Review Comment:
   Random change?



##########
src/relay/ir/dataflow_matcher_impl.h:
##########
@@ -39,10 +41,21 @@ namespace relay {
 
 class DFPatternMatcher : public DFPatternFunctor<bool(const DFPattern&, const Expr&)> {
  public:
-  explicit DFPatternMatcher(const Expr& root_expr) : expr_graph_(CreateIndexedGraph(root_expr)) {}
+  explicit DFPatternMatcher(const IndexedGraph<Expr>* expr_graph) : expr_graph_(expr_graph) {}
   bool Match(const DFPattern& pattern, const Expr& expr);
   Map<DFPattern, Array<Expr>> GetMemo() { return Map<DFPattern, Array<Expr>>(memo_); }
-  const IndexedGraph<Expr> expr_graph_;
+
+  const IndexedGraph<Expr>::Node* expr_to_node(const Expr& expr) const {
+    return expr_graph_->item_to_node(expr);
+  }
+  const IndexedGraph<Expr>::Node* index_to_node(size_t index) const {
+    return expr_graph_->index_to_node(index);
+  }
+  size_t size() const { return expr_graph_->size(); }
+  const std::unordered_map<DFPattern, Array<Expr>, ObjectPtrHash, ObjectPtrEqual>& memo() const {
+    return memo_;
+  }
+  const IndexedGraph<Expr>& dataflow_graph() const { return *expr_graph_; }

Review Comment:
   The mixed terminology here is quite confusing (Indexed/DataFlow/Expr graph). I think it should be aligned around one convention.



##########
src/runtime/contrib/tensorrt/tensorrt_ops.cc:
##########
@@ -67,7 +67,7 @@ nvinfer1::ITensor* TensorRTOpConverter::Transpose(TensorRTOpConverterParams* par
     // Batch dimension cannot be modified.
     ICHECK_EQ(input->getDimensions().nbDims, order.size() - 1);
     ICHECK_EQ(order[0], 0);
-    for (size_t i = 0; i < order.size(); ++i) {
+    for (size_t i = 0; i + 1 < order.size(); ++i) {

Review Comment:
   Is this needed because of the changes to indexed_graph (do the tests fail without it)?



##########
include/tvm/relay/expr_functor.h:
##########
@@ -240,6 +240,8 @@ class MixedModeVisitor : public ::tvm::relay::ExprVisitor {
    */
   explicit MixedModeVisitor(int visit_limit = 1);
 
+  using ExprVisitor::VisitExpr_;
+

Review Comment:
   What's this for?



##########
src/relay/ir/expr.cc:
##########
@@ -27,6 +27,26 @@
 
 namespace tvm {
 
+GlobalVar WithFields(GlobalVar global_var, Optional<String> opt_name_hint, Optional<Type> opt_type,

Review Comment:
   I'd rather punt all these WithFields changes into a separate PR. I realise there's some historic testing debt that makes that less appealing, but maybe we can be flexible in that regard and open an issue to solve the problem later.



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