[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r529015930



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -61,20 +67,27 @@ class AnnotateTargetRewriter : public ExprRewriter {
   std::pair> AnnotateArgs(const Array& args,
const std::string& target = 
"") {
 std::string ref_target = "";
+Array compiler_begins;
 Array compiler_ends;
 for (auto arg : args) {
-  std::string arg_target = "default";
+  std::string arg_target = default_target;
   const CallNode* call = arg.as();
 
   if (call && call->op == CompilerBeginOp()) {
 // Argument is already compiler begin node meaning that this is not 
the first time
 // running this pass, so we simply remove it and will add a new one 
later.
 ICHECK_EQ(call->args.size(), 1U);
+// Do not alter existing annotation if not default
+if (default_target != call->attrs.as()->compiler) {
+  compiler_begins.push_back(arg);
+} else {
+  // Remove default
+  compiler_ends.push_back(call->args[0]);
+}

Review comment:
   The proposal is likely to be more flexible. But I am afraid that 
introducing an externally defined method for doing pruning also means 
introducing a more elaborate checker for IR as it should be clearly stated what 
is the correctly pruned graph and what is not.





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528967519



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -61,20 +67,27 @@ class AnnotateTargetRewriter : public ExprRewriter {
   std::pair> AnnotateArgs(const Array& args,
const std::string& target = 
"") {
 std::string ref_target = "";
+Array compiler_begins;
 Array compiler_ends;
 for (auto arg : args) {
-  std::string arg_target = "default";
+  std::string arg_target = default_target;
   const CallNode* call = arg.as();
 
   if (call && call->op == CompilerBeginOp()) {
 // Argument is already compiler begin node meaning that this is not 
the first time
 // running this pass, so we simply remove it and will add a new one 
later.
 ICHECK_EQ(call->args.size(), 1U);
+// Do not alter existing annotation if not default
+if (default_target != call->attrs.as()->compiler) {
+  compiler_begins.push_back(arg);
+} else {
+  // Remove default
+  compiler_ends.push_back(call->args[0]);
+}

Review comment:
   Current contract for AnnotateTarget assumes that for non-call op to be 
promoted under the target of its arguments it should have all the arguments 
annotated with the same target. If I am not mistaken, the targets can be added 
at the run-time, and theoretically the situation is possible when after several 
subsequent invocations of AnnotateTarget a non-call operation will be annotated 
as default and will not be promoted even when all its arguments have same 
target. To prevent this the default annotations always removed while specific 
targets are preserved. However I am agree that the explanation is a bit 
far-fetched.
   
   As for the second part of your comment, do I understand correctly that the 
idea is not to alter the AnnotateTarget and instead do the removal of 
annotations from non-call ops in a subsequent separate pass? I am not sure I 
quite understand the definition of "simple" and the concept of 
PruneSimpleRegions as a whole. Could you elaborate this point a bit more, 
please?





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528871200



##
File path: tests/python/relay/test_pass_annotate_target.py
##
@@ -212,9 +212,12 @@ def after():
 mod = tvm.IRModule.from_expr(f)
 return mod
 
-result = transform.AnnotateTarget("test")(before())
-expected = transform.InferType()(after())
-assert tvm.ir.structural_equal(expected, result)
+for annotate_non_call_ops in [False, True]:
+result = transform.AnnotateTarget("test", 
annotate_non_call_ops)(before())
+print("Result", annotate_non_call_ops, "\n", result, "\n")

Review comment:
   Done. Removed





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528787976



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -146,13 +179,19 @@ class AnnotateTargetRewriter : public ExprRewriter {
   // Bypass compiler begin due to lack of target information. It will be 
processed
   // when the following op handling arguments.
   ICHECK_EQ(pre->args.size(), 1U);
-  return post.as()->args[0];
+  // Preserve annotations
+  return post;
 } else if (op_node && pre->op == CompilerEndOp()) {
   // Override compiler end with the new target.
   ICHECK_EQ(pre->args.size(), 1U);
   auto input_expr = post.as()->args[0];
+  // Already annotated. Recover target
+  if (op_expr_to_target_.find(input_expr) == op_expr_to_target_.end()) {
+op_expr_to_target_[input_expr] = 
post.as()->attrs.as()->compiler;
+  }

Review comment:
   If this is not the first invocation of the pass this branch supposed to 
restore targets from already annotated ops.





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528788103



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -93,18 +106,20 @@ class AnnotateTargetRewriter : public ExprRewriter {
   if (ref_target == "") {
 ref_target = arg_target;
   } else if (ref_target != arg_target) {
-ref_target = "default";
+ref_target = default_target;
   }
 }
 
 // Determine compiler begin target.
 std::string op_target = (target == "") ? ref_target : target;
 
-Array compiler_begins;
-for (const auto& end : compiler_ends) {
-  compiler_begins.push_back(InsertAnnotation(end, op_target, 
make_begin_op));
+if (ref_target != "") {
+  for (const auto& end : compiler_ends) {
+compiler_begins.push_back(InsertAnnotation(end, op_target, 
make_begin_op));
+  }
+} else {
+  return {op_target, args};

Review comment:
   I do not understand you comment. please elaborate.





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528787976



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -146,13 +179,19 @@ class AnnotateTargetRewriter : public ExprRewriter {
   // Bypass compiler begin due to lack of target information. It will be 
processed
   // when the following op handling arguments.
   ICHECK_EQ(pre->args.size(), 1U);
-  return post.as()->args[0];
+  // Preserve annotations
+  return post;
 } else if (op_node && pre->op == CompilerEndOp()) {
   // Override compiler end with the new target.
   ICHECK_EQ(pre->args.size(), 1U);
   auto input_expr = post.as()->args[0];
+  // Already annotated. Recover target
+  if (op_expr_to_target_.find(input_expr) == op_expr_to_target_.end()) {
+op_expr_to_target_[input_expr] = 
post.as()->attrs.as()->compiler;
+  }

Review comment:
   Removed IF





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528781372



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -203,16 +242,18 @@ class AnnotateTargetRewriter : public ExprRewriter {
 }
   }
 }
-supported_targets.push_back("default");  // Make default as the last 
option.
-
+supported_targets.push_back(default_target);  // Make default as the last 
option.
+// Visit and mutate arguments after the target of this op has been 
determined.
+Call post_call = Downcast(post);
+if (pre->op->IsInstance()) {

Review comment:
   The test case is 
tests/python/relay/test_pass_annotate_target.py::test_while_let

##
File path: src/relay/transforms/annotate_target.cc
##
@@ -203,16 +242,18 @@ class AnnotateTargetRewriter : public ExprRewriter {
 }
   }
 }
-supported_targets.push_back("default");  // Make default as the last 
option.
-
+supported_targets.push_back(default_target);  // Make default as the last 
option.
+// Visit and mutate arguments after the target of this op has been 
determined.
+Call post_call = Downcast(post);
+if (pre->op->IsInstance()) {

Review comment:
   The test case is 
`tests/python/relay/test_pass_annotate_target.py::test_while_let`





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528775498



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -161,8 +200,9 @@ class AnnotateTargetRewriter : public ExprRewriter {
   const CallNode* first_arg_call = pre->args[0].as();
   if (first_arg_call && first_arg_call->op == CompilerBeginOp()) {
 std::string arg_target = 
first_arg_call->attrs.as()->compiler;
-if (arg_target != "default") {
-  supported_targets.push_back(arg_target);
+if (arg_target != default_target) {
+  // annotated already
+  return post;

Review comment:
   Here it is peeking first arg and if it is already annotated with 
non-default target it returns the node untouched, preserving the target.





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528759768



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -128,14 +143,32 @@ class AnnotateTargetRewriter : public ExprRewriter {
  * \return An annotated and target-propagated relay expression.
  */
 Expr new_expr = expr;
-if (op_expr_to_target_.find(expr) != op_expr_to_target_.end() && 
FreeVars(expr).size() != 0) {
-  new_expr = InsertAnnotation(expr, op_expr_to_target_[expr], make_end_op);
-  op_expr_to_target_[new_expr] = op_expr_to_target_[expr];
+const CallNode* call = expr.as();
+if (op_expr_to_target_.find(expr) != op_expr_to_target_.end()) {
+  // Check whether expr has args, if not - do not insert compiler_end.

Review comment:
   The comment related to this part `(call && !call->args.empty()))` of the 
condition





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528697334



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -128,14 +143,32 @@ class AnnotateTargetRewriter : public ExprRewriter {
  * \return An annotated and target-propagated relay expression.
  */
 Expr new_expr = expr;
-if (op_expr_to_target_.find(expr) != op_expr_to_target_.end() && 
FreeVars(expr).size() != 0) {
-  new_expr = InsertAnnotation(expr, op_expr_to_target_[expr], make_end_op);
-  op_expr_to_target_[new_expr] = op_expr_to_target_[expr];
+const CallNode* call = expr.as();
+if (op_expr_to_target_.find(expr) != op_expr_to_target_.end()) {
+  // Check whether expr has args, if not - do not insert compiler_end.
+  expr->IsInstance();

Review comment:
   Removed.





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528640875



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -61,20 +67,27 @@ class AnnotateTargetRewriter : public ExprRewriter {
   std::pair> AnnotateArgs(const Array& args,
const std::string& target = 
"") {
 std::string ref_target = "";
+Array compiler_begins;
 Array compiler_ends;
 for (auto arg : args) {
-  std::string arg_target = "default";
+  std::string arg_target = default_target;
   const CallNode* call = arg.as();
 
   if (call && call->op == CompilerBeginOp()) {
 // Argument is already compiler begin node meaning that this is not 
the first time
 // running this pass, so we simply remove it and will add a new one 
later.
 ICHECK_EQ(call->args.size(), 1U);
+// Do not alter existing annotation if not default
+if (default_target != call->attrs.as()->compiler) {
+  compiler_begins.push_back(arg);
+} else {
+  // Remove default
+  compiler_ends.push_back(call->args[0]);
+}

Review comment:
   It removes default (but preserve non-default) annotations in case if it 
is not first invocation of the pass





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528595071



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -113,18 +118,26 @@ class AnnotateTargetRewriter : public ExprRewriter {
 std::vector supported_targets;
 
 auto op_node = pre->op.as();
-
 // This graph has annotations, meaning that this is not the first time 
running this pass.
 if (op_node && pre->op == CompilerBeginOp()) {
   // Bypass compiler begin due to lack of target information. It will be 
processed
   // when the following op handling arguments.
   CHECK_EQ(pre->args.size(), 1U);
-  return post.as()->args[0];
+
+  std::string begin_target = pre->attrs.as()->compiler;
+  if ("" == begin_target || begin_target == DEFAULT_TARGET_NAME)

Review comment:
   refactored





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528593419



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -62,19 +65,22 @@ class AnnotateTargetRewriter : public ExprRewriter {
const std::string& target = 
"") {
 std::string ref_target = "";
 Array compiler_ends;
+Array compiler_begins;
 for (auto arg : args) {
-  std::string arg_target = "default";
+  std::string arg_target = DEFAULT_TARGET_NAME;
   const CallNode* call = arg.as();
-
   if (call && call->op == CompilerBeginOp()) {
 // Argument is already compiler begin node meaning that this is not 
the first time
 // running this pass, so we simply remove it and will add a new one 
later.
 CHECK_EQ(call->args.size(), 1U);
 const CallNode* end = call->args[0].as();
-if (end->op == CompilerEndOp()) {
+if (end && end->op == CompilerEndOp()) {
   arg_target = end->attrs.as()->compiler;
 }
-compiler_ends.push_back(call->args[0]);
+if (arg_target == "" || arg_target == DEFAULT_TARGET_NAME)
+  compiler_ends.push_back(call->args[0]);
+else
+  compiler_begins.push_back(arg);

Review comment:
   refactored





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528593279



##
File path: src/relay/transforms/annotate_target.cc
##
@@ -113,18 +118,26 @@ class AnnotateTargetRewriter : public ExprRewriter {
 std::vector supported_targets;
 
 auto op_node = pre->op.as();
-
 // This graph has annotations, meaning that this is not the first time 
running this pass.
 if (op_node && pre->op == CompilerBeginOp()) {
   // Bypass compiler begin due to lack of target information. It will be 
processed
   // when the following op handling arguments.
   CHECK_EQ(pre->args.size(), 1U);
-  return post.as()->args[0];
+
+  std::string begin_target = pre->attrs.as()->compiler;
+  if ("" == begin_target || begin_target == DEFAULT_TARGET_NAME)
+return post.as()->args[0];
+
+  return post;
 } else if (op_node && pre->op == CompilerEndOp()) {
   // Override compiler end with the new target.
   CHECK_EQ(pre->args.size(), 1U);
   auto input_expr = post.as()->args[0];
   CHECK(op_expr_to_target_.find(input_expr) != op_expr_to_target_.end());
+
+  std::string begin_target = pre->attrs.as()->compiler;
+  if ("" != begin_target && begin_target != DEFAULT_TARGET_NAME) return 
post;

Review comment:
   Refactored





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528585798



##
File path: tests/python/relay/test_pass_annotate_target.py
##
@@ -598,7 +794,7 @@ def after():
 if __name__ == "__main__":
 test_extern_dnnl()
 test_composite_function()
-# test_extern_dnnl_mobilenet()
+test_extern_dnnl_mobilenet()

Review comment:
   fixed





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528586663



##
File path: python/tvm/relay/transform/transform.py
##
@@ -666,7 +666,7 @@ def PartitionGraph():
 return _ffi_api.PartitionGraph()
 
 
-def AnnotateTarget(targets):
+def AnnotateTarget(targets, default_tuples=False):

Review comment:
   refactored as include_non_call_ops, defaults to True





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:
us...@infra.apache.org

[GitHub] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass

2020-11-23 Thread GitBox 


d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528586189



##
File path: tests/python/relay/test_pass_annotate_target.py
##
@@ -327,6 +327,42 @@ def after():
 assert tvm.ir.structural_equal(expected, result)
 
 
+def test_tuple_two_targets():
+"""Tests whether the TupleNode is promoted to previously annotatated 
operation or is excluded."""
+target_relu = "relu_target"
+target_maximum = "maximum_target"
+target_default = "default"
+
+@tvm.ir.register_op_attr("nn.relu", "target." + target_relu)
+def relu(attrs, args):  # pylint: disable=unused-variable
+return True
+
+@tvm.ir.register_op_attr("maximum", "target." + target_maximum)
+def maximum(attrs, args):  # pylint: disable=unused-variable
+return True
+
+def before():
+a = relay.var("a", shape=(10, 5))
+b = relay.var("b", shape=(10, 5))
+r = relay.nn.relu(b)
+t1 = relay.Tuple((r, r))
+r2 = relay.nn.relu(t1)
+m = relay.maximum(a, b)
+t2 = relay.Tuple((m, r2))
+f = relay.Function([a, b], t2)
+return tvm.IRModule.from_expr(f)
+
+for default_tuples, parts in [(True, 3), (False, 2)]:
+result = before()
+result = transform.AnnotateTarget([target_relu], 
default_tuples)(result)
+result = transform.AnnotateTarget([target_maximum], True)(result)
+result = transform.MergeCompilerRegions()(result)

Review comment:
   removed





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:
us...@infra.apache.org