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/11/20 21:23:12 UTC
[GitHub] [incubator-tvm] comaniac commented on a change in pull request #6655: [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass
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