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/10/09 19:20:13 UTC
[GitHub] [incubator-tvm] d-smirnov opened a new pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
d-smirnov opened a new pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655
This PR adds "default_tuples" parameter to AnnotateTarget pass. When set (default_tuples=True) it prevents tuples to be promoted to previously annotated operations. This is useful in case if you are not running MergeCompilerRegions pass after AnnotateTarget.
The PR resolves the issue reported here: https://discuss.tvm.apache.org/t/arm-compute-library-segv-with-inception-v1-squeezenet/7985/8
@comaniac @manupa-arm @mbaret
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r527961699
##########
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:
This test is skipped.
##########
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:
Remove unnecessary prints. Ditto to others.
##########
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<CallNode>();
+ 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<RefWriteNode>();
Review comment:
No effect?
##########
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<Call>(post);
+ if (pre->op->IsInstance<VarNode>()) {
Review comment:
Could you elaborate why a CallNode may have a VarNode as its op?
##########
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<Expr> 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:
This makes L188 unreachable?
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -61,20 +67,27 @@ class AnnotateTargetRewriter : public ExprRewriter {
std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
const std::string& target = "") {
std::string ref_target = "";
+ Array<Expr> compiler_begins;
Array<Expr> compiler_ends;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = default_target;
const CallNode* call = arg.as<CallNode>();
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<CompilerAttrs>()->compiler) {
+ compiler_begins.push_back(arg);
+ } else {
+ // Remove default
+ compiler_ends.push_back(call->args[0]);
+ }
Review comment:
I didn't quite get this logic. Could you elaborate why?
##########
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<CallNode>();
if (first_arg_call && first_arg_call->op == CompilerBeginOp()) {
std::string arg_target = first_arg_call->attrs.as<CompilerAttrs>()->compiler;
- if (arg_target != "default") {
- supported_targets.push_back(arg_target);
+ if (arg_target != default_target) {
+ // annotated already
+ return post;
Review comment:
Looks like you remove the feature that considers the target in existing annotation nodes?
##########
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<CallNode>();
+ 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:
Cannot connect this comment to the following logic. Could you elaborate?
##########
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<CallNode>()->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<CallNode>()->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<CallNode>()->attrs.as<CompilerAttrs>()->compiler;
+ }
Review comment:
Looks like you don't need the IF? Even `input_expr` is already in `op_expr_to_target_`, you can still override it, as suggested by the comment in L188. Accordingly, if you will override the target, you need `InsertAnnotation`.
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#issuecomment-706369034
Thanks for the PR. I have a hard deadline this weekend and will review it after next Tuesday.
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<CallNode>()->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<CallNode>()->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<CallNode>()->attrs.as<CompilerAttrs>()->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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r510715272
##########
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:
I think what @zhiics means is all the overridden [Rewrite_]s of annotate_target except CallNode -- where Tuple is just one of them.
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<CallNode>();
+ 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:
users@infra.apache.org
[GitHub] [incubator-tvm] mbaret commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504073541
##########
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)
Review comment:
Yeah, it's just confusing that we have two different ways to achieve the same result. Given multiple runs are supported, it seems like we don't need to complexity of supporting a list of targets.
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov commented on pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#issuecomment-727622983
> Hey @d-smirnov have you tried with this change? #6912
Rebased, but it did not make any difference. Still fail on the check `src/relay/backend/compile_engine.cc:170`
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r529032585
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -61,20 +67,27 @@ class AnnotateTargetRewriter : public ExprRewriter {
std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
const std::string& target = "") {
std::string ref_target = "";
+ Array<Expr> compiler_begins;
Array<Expr> compiler_ends;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = default_target;
const CallNode* call = arg.as<CallNode>();
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<CompilerAttrs>()->compiler) {
+ compiler_begins.push_back(arg);
+ } else {
+ // Remove default
+ compiler_ends.push_back(call->args[0]);
+ }
Review comment:
IMHO, it should be fine, because we have abstracted the region to a separate data structure called "AnnotatedRegion". If we pass an AnnotatedRegion to PruneSimpleRegions, we just work on the region level. We can add an API such as "ChangeRegionTarget" to AnnotatedRegion that changes all compiler attributes of the nodes in that region. In this case, PruneSimpleRegion could just do (in Pseudo code):
```
for region in regions:
if region.target != "default" and is_simple_region(region):
region.change_region_target("default")
```
and one implementation of "is_simple_region" coud be like:
```
def is_simple_region(region):
class Checker(ExprVisitor):
def __init__(self):
self.is_simple = True
def visit(self, call):
self.is_simple = False
super(self).visit(call)
checker = Checker()
for out in region.outs:
checker.visit(out)
return checker.is_simple
```
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<CallNode>();
+ 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<RefWriteNode>();
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:
users@infra.apache.org
[GitHub] [incubator-tvm] d-smirnov commented on pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov commented on pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#issuecomment-727605634
@tqchen Hi, I am trying to compile Mobilnet (after expanded this patch to all non-call ops, not committed yet) and encountered a check on src/relay/backend/compile_engine.cc:170
`ICHECK(op->is_scalar());`
The ConstantNode it fails is of TensorType: TensorType([1, 1, 1024, 1024], float32), and contains runtime.NDArray(0x74f0370) of corresponding shape (1x1x1024x1024). Could you explain the purpose of the check and should the check be extended to also accomodate ConstantNodes of TensorType. I am also interested to know the techniques to trace when and why this ConstantNode appeared.
Thank you. cc: @comaniac
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504052671
##########
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)
Review comment:
There is the test_multiple_runs() there, I think its run multiple times and checks for equivalency as if it ran a single time using two targets.
----------------------------------------------------------------
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] [incubator-tvm] manupa-arm commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r510795269
##########
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:
I think if skip_non_call_ops=True, we should not annotate Lets -- even with defaults. Thus, the behavior is not the same.
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov commented on pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#issuecomment-731392966
Updated. Please take a look @comaniac, @mbaret, @manupa-arm
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<CallNode>()->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<CallNode>()->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<CallNode>()->attrs.as<CompilerAttrs>()->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:
users@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
Posted by GitBox <gi...@apache.org>.
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<Expr> 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:
users@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
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528879637
##########
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<Expr> 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:
nvm. I figured it out.
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504078734
##########
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)
Review comment:
List of targets serves for the different purpose. List of target allows us to have a whole picture during the annotation. We can determine which target an op should be annotated by considering the region size, for example. As a result, I personally prefer to have a list of targets instead of multiple runs if we could. However, given the scenario you provided, which two targets require different graphs, it seems inevitable.
----------------------------------------------------------------
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] [incubator-tvm] mbaret commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504054803
##########
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)
Review comment:
Oh OK, makes me wonder why we're carrying the complexity of allowing multiple targets then, but that's a question for another time...
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<std::string> supported_targets;
auto op_node = pre->op.as<OpNode>();
-
// 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<CallNode>()->args[0];
+
+ std::string begin_target = pre->attrs.as<CompilerAttrs>()->compiler;
+ if ("" == begin_target || begin_target == DEFAULT_TARGET_NAME)
+ return post.as<CallNode>()->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<CallNode>()->args[0];
CHECK(op_expr_to_target_.find(input_expr) != op_expr_to_target_.end());
+
+ std::string begin_target = pre->attrs.as<CompilerAttrs>()->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:
users@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
Posted by GitBox <gi...@apache.org>.
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<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
const std::string& target = "") {
std::string ref_target = "";
+ Array<Expr> compiler_begins;
Array<Expr> compiler_ends;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = default_target;
const CallNode* call = arg.as<CallNode>();
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<CompilerAttrs>()->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:
users@infra.apache.org
[GitHub] [incubator-tvm] d-smirnov edited a comment on pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov edited a comment on pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#issuecomment-727622983
> Hey @d-smirnov have you tried with this change? #6912
@trevor-m Rebased, but it did not make any difference. Still fail on the check `src/relay/backend/compile_engine.cc:170`
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<CallNode>();
if (first_arg_call && first_arg_call->op == CompilerBeginOp()) {
std::string arg_target = first_arg_call->attrs.as<CompilerAttrs>()->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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r503308197
##########
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<Expr> compiler_ends;
+ Array<Expr> compiler_begins;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = DEFAULT_TARGET_NAME;
const CallNode* call = arg.as<CallNode>();
-
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<CallNode>();
- if (end->op == CompilerEndOp()) {
+ if (end && end->op == CompilerEndOp()) {
arg_target = end->attrs.as<CompilerAttrs>()->compiler;
}
- compiler_ends.push_back(call->args[0]);
+ if (arg_target == "" || arg_target == DEFAULT_TARGET_NAME)
Review comment:
brackets (here and else as well) ?
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r510764096
##########
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:
For example, current implementation of `Let` in relay does not accept anything other than `Var` as its first parameter. Effectively, it cannot be annotated with anything but `"default"`. So for `Let` these changes would not make any difference.
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504066854
##########
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)
Review comment:
@mbaret see the description in https://github.com/apache/incubator-tvm/pull/5277. I recall this is the requirement from you or the other ARM folk. In that case you need to annotate differernt targets before and after QNN passes.
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528914573
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -61,20 +67,27 @@ class AnnotateTargetRewriter : public ExprRewriter {
std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
const std::string& target = "") {
std::string ref_target = "";
+ Array<Expr> compiler_begins;
Array<Expr> compiler_ends;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = default_target;
const CallNode* call = arg.as<CallNode>();
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<CompilerAttrs>()->compiler) {
+ compiler_begins.push_back(arg);
+ } else {
+ // Remove default
+ compiler_ends.push_back(call->args[0]);
+ }
Review comment:
My question was about the algorithm. I don't understand why we "remove default (but preserve non-default) annotations in case if it is not first invocation of the pass". Almost all my comments related to this question.
Meanwhile, PR derives another AnnotateTarget implementation to deal with the case that `include_non_call_ops=False`, so you are more like making a new pass and use `include_non_call_ops` to dispatch the one being used. Combining my previous concern about keeping AnnotateTarget simple and straightforward, I would suggest making it a separate pass. For example, we could name the new pass "PruneSimpleRegions" and use it as following:
1. (MergeComposiste) -> AnnotateTarget -> PruneSimpleRegions -> PartitionGraph
2. (MergeComposiste) -> AnnotateTarget -> MergeCompilerRegions -> PruneSimpleRegions -> PartitionGraph
The first case could achieve your goal, and it could also serve the second case to prune subgraphs (compiler regions) after merging. If we take one step further, we could even let users provide a lambda function to determine if a region is "simple", so that it can be customized for different backends such as TensorRT. The implementation of this pass could also be much simpler and I could even help with that if you prefer.
What do you think?
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r509363155
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -40,11 +40,14 @@ static const PackedFunc* make_begin_op =
static const PackedFunc* make_end_op =
runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+#define DEFAULT_TARGET_NAME ("default")
Review comment:
Done
----------------------------------------------------------------
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] [incubator-tvm] mbaret commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
mbaret commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504044962
##########
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:
I think we should be testing the failure case we saw, which was when MergeCompilerRegions wasn't run.
##########
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)
Review comment:
Perhaps @comaniac can comment here, but my understanding was that AnnotateTarget wasn't meant to be run multiple times like this.
----------------------------------------------------------------
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] [incubator-tvm] comaniac commented on pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#issuecomment-707867942
This solution looks too ad hoc to me. From the semantic of the changes, I think this PR can be generalized to "improve the annotate target pass to annotate non-call ops to default". Accordingly, the API could be like
```python
AnnotateTarget(targets, skip_non_call_ops=False)(mod) # I am bad at naming but it is the point
```
cc @zhiics
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
const std::string& target = "") {
std::string ref_target = "";
+ Array<Expr> compiler_begins;
Array<Expr> compiler_ends;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = default_target;
const CallNode* call = arg.as<CallNode>();
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<CompilerAttrs>()->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:
users@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
Posted by GitBox <gi...@apache.org>.
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<Expr> compiler_ends;
+ Array<Expr> compiler_begins;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = DEFAULT_TARGET_NAME;
const CallNode* call = arg.as<CallNode>();
-
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<CallNode>();
- if (end->op == CompilerEndOp()) {
+ if (end && end->op == CompilerEndOp()) {
arg_target = end->attrs.as<CompilerAttrs>()->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:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r528990741
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -61,20 +67,27 @@ class AnnotateTargetRewriter : public ExprRewriter {
std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
const std::string& target = "") {
std::string ref_target = "";
+ Array<Expr> compiler_begins;
Array<Expr> compiler_ends;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = default_target;
const CallNode* call = arg.as<CallNode>();
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<CompilerAttrs>()->compiler) {
+ compiler_begins.push_back(arg);
+ } else {
+ // Remove default
+ compiler_ends.push_back(call->args[0]);
+ }
Review comment:
Thanks for the explanation. I think I am now clearer about the first part, but we might need to improve the code comments a lot to make it clear to everyone reading this pass.
For the second part, your understanding is correct. As you mentioned, the definition of "simple" is vague, but it's also more general. Since every target needs to specify their own transform pipeline (e.g., https://github.com/apache/incubator-tvm/blob/main/python/tvm/relay/op/contrib/arm_compute_lib.py#L44), we can every BYOC target can define their own "simple". For example, the definition of "simple" in ACL could be a subgraph with non-call ops; the definition of "simple" in TensorRT could be a subgraph without MAC ops. It means something like
```python
seq = tvm.transform.Sequential(
[
transform.InferType(),
transform.MergeComposite(arm_compute_lib_pattern_table()),
transform.AnnotateTarget("arm_compute_lib"),
transform.PruneSimpleRegions(tvm._ffi.get_global_func("relay.ext.arm_compute_lib.prune")),
transform.PartitionGraph(),
]
)
```
where `tvm._ffi.get_global_func("relay.ext.arm_compute_lib.prune")` is a packed function in C++ that accepts an `AnnotatedRegion` and outputs a boolean, indicating whether the region should be pruned or 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:
users@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
Posted by GitBox <gi...@apache.org>.
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<std::string> supported_targets;
auto op_node = pre->op.as<OpNode>();
-
// 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<CallNode>()->args[0];
+
+ std::string begin_target = pre->attrs.as<CompilerAttrs>()->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:
users@infra.apache.org
[GitHub] [incubator-tvm] manupa-arm commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r503299324
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -40,11 +40,14 @@ static const PackedFunc* make_begin_op =
static const PackedFunc* make_end_op =
runtime::Registry::Get("relay.op.annotation._make.compiler_end");
+#define DEFAULT_TARGET_NAME ("default")
Review comment:
Can we use constexpr or const specifier instead ? -- to scope the parameter.
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -113,18 +118,26 @@ class AnnotateTargetRewriter : public ExprRewriter {
std::vector<std::string> supported_targets;
auto op_node = pre->op.as<OpNode>();
-
// 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<CallNode>()->args[0];
+
+ std::string begin_target = pre->attrs.as<CompilerAttrs>()->compiler;
+ if ("" == begin_target || begin_target == DEFAULT_TARGET_NAME)
Review comment:
brackets
##########
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<Expr> compiler_ends;
+ Array<Expr> compiler_begins;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = DEFAULT_TARGET_NAME;
const CallNode* call = arg.as<CallNode>();
-
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<CallNode>();
- if (end->op == CompilerEndOp()) {
+ if (end && end->op == CompilerEndOp()) {
arg_target = end->attrs.as<CompilerAttrs>()->compiler;
}
- compiler_ends.push_back(call->args[0]);
+ if (arg_target == "" || arg_target == DEFAULT_TARGET_NAME)
Review comment:
brackets (here and else as well) ?, did this pass through clang-format ?
##########
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<Expr> compiler_ends;
+ Array<Expr> compiler_begins;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = DEFAULT_TARGET_NAME;
const CallNode* call = arg.as<CallNode>();
-
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<CallNode>();
- if (end->op == CompilerEndOp()) {
+ if (end && end->op == CompilerEndOp()) {
arg_target = end->attrs.as<CompilerAttrs>()->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:
Maybe its worth adding a comment, this is to not touch (non-empty and non-default) annotation. Took sometime figure out the unusual compiler_begins append at this location of the code. Just a suggestion.
##########
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)
+ result = transform.PartitionGraph()(result)
+ assert parts == len(
+ list(filter(lambda _: "target" in _.name_hint, result.get_global_vars()))
Review comment:
While the number of partitions gives an indication whether tuples are going to into partition, don't you think checking explicitly for sub-graphs of tuples make sense?
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -113,18 +118,26 @@ class AnnotateTargetRewriter : public ExprRewriter {
std::vector<std::string> supported_targets;
auto op_node = pre->op.as<OpNode>();
-
// 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<CallNode>()->args[0];
+
+ std::string begin_target = pre->attrs.as<CompilerAttrs>()->compiler;
+ if ("" == begin_target || begin_target == DEFAULT_TARGET_NAME)
Review comment:
This leaves the above comment out of place; Now it seems we only bypass compiler begins if its default or empty. Thus, move the comment here and change accordingly. Anyway, is there a reason why we are bypassing "default" compiler_begins here and adding them back in AnnotateArgs ? -- If we did not bypass them, we dont need to add them back (in line 80) ?
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -113,18 +118,26 @@ class AnnotateTargetRewriter : public ExprRewriter {
std::vector<std::string> supported_targets;
auto op_node = pre->op.as<OpNode>();
-
// 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<CallNode>()->args[0];
+
+ std::string begin_target = pre->attrs.as<CompilerAttrs>()->compiler;
+ if ("" == begin_target || begin_target == DEFAULT_TARGET_NAME)
+ return post.as<CallNode>()->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<CallNode>()->args[0];
CHECK(op_expr_to_target_.find(input_expr) != op_expr_to_target_.end());
+
+ std::string begin_target = pre->attrs.as<CompilerAttrs>()->compiler;
+ if ("" != begin_target && begin_target != DEFAULT_TARGET_NAME) return post;
Review comment:
Same question here as well; reason for treating already put "default" annotation as they were empty ?
----------------------------------------------------------------
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] [incubator-tvm] trevor-m commented on pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
trevor-m commented on pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#issuecomment-727605945
> @tqchen Hi, I am trying to compile Mobilnet (after expanded this patch to all non-call ops, not committed yet) and encountered a check on src/relay/backend/compile_engine.cc:170
> `ICHECK(op->is_scalar());`
>
> The ConstantNode it fails is of TensorType: TensorType([1, 1, 1024, 1024], float32), and contains runtime.NDArray(0x74f0370) of corresponding shape (1x1x1024x1024). Could you explain the purpose of the check and should the check be extended to also accomodate ConstantNodes of TensorType. I am also interested to know the techniques to trace when and why this ConstantNode appeared.
>
> Thank you. cc: @comaniac
Hey @d-smirnov have you tried with this change? https://github.com/apache/incubator-tvm/pull/6912
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r510442028
##########
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:
Could you elaborate the list of non-call nodes that should have this feature along with the Tuple, 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:
users@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
Posted by GitBox <gi...@apache.org>.
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<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
const std::string& target = "") {
std::string ref_target = "";
+ Array<Expr> compiler_begins;
Array<Expr> compiler_ends;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = default_target;
const CallNode* call = arg.as<CallNode>();
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<CompilerAttrs>()->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:
users@infra.apache.org
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504063626
##########
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)
Review comment:
I remembered there was a scenario that needs multiple runs of target annotation, but can't remember details at this moment.
----------------------------------------------------------------
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] [incubator-tvm] zhiics commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r504131348
##########
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:
This API sounds quite ad-hoc to me. `Tuple` is just one of the nodes that can take the same target from its inputs/args. Incrementally adding parameters to handle different nodes would make the API bulky and code less readable, IMHO.
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
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<Call>(post);
+ if (pre->op->IsInstance<VarNode>()) {
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<Call>(post);
+ if (pre->op->IsInstance<VarNode>()) {
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:
users@infra.apache.org
[GitHub] [incubator-tvm] trevor-m commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
trevor-m commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r503524595
##########
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:
Perhaps `include_tuples=True` is more descriptive?
----------------------------------------------------------------
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] [incubator-tvm] d-smirnov commented on a change in pull request #6655: [BYOC] Added default_tuples parameter to AnnotateTarget pass
Posted by GitBox <gi...@apache.org>.
d-smirnov commented on a change in pull request #6655:
URL: https://github.com/apache/incubator-tvm/pull/6655#discussion_r509362453
##########
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<Expr> compiler_ends;
+ Array<Expr> compiler_begins;
for (auto arg : args) {
- std::string arg_target = "default";
+ std::string arg_target = DEFAULT_TARGET_NAME;
const CallNode* call = arg.as<CallNode>();
-
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<CallNode>();
- if (end->op == CompilerEndOp()) {
+ if (end && end->op == CompilerEndOp()) {
arg_target = end->attrs.as<CompilerAttrs>()->compiler;
}
- compiler_ends.push_back(call->args[0]);
+ if (arg_target == "" || arg_target == DEFAULT_TARGET_NAME)
Review comment:
Done
----------------------------------------------------------------
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