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/04/08 06:58:31 UTC
[GitHub] [incubator-tvm] comaniac opened a new pull request #5277: [BYOC]
Refine AnnotateTarget and MergeCompilerRegion Passes
comaniac opened a new pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277
Since we are working on TRT support with BYOC infra, we added some features and fixed some bugs in AnnotateTarget and MergeCompilerRegion passes. Here are the change logs:
**Bug Fix**
- Fixed type propagation and tuple bug in AnnotateTarget pass (@trevor-m ).
- Corrected the namespace of MergeCompilerRegion.
**Support Multiple Targets**
- Add multiple target support to AnnotateTarget. Now it takes either a target string or a list of targets. AnnotateTarget will invoke target specific op-based rule functions in order.
- Accordingly, now regions in AnnotateRegionSet also have target attribute.
**Unified Pass Input/Output**
- Previously the output graph of AnnotateTarget annotates the ops for external target, but the input graph of AnnotateRegionSet requires every op to be annotated. Now both AnnotateTarget and MergeCompilerRegion passes output a graph with all ops annotated. Ops that will be handeled by TVM will also be annotated with target "default".
- Accordingly, we don't need AnnotateRestDefault anymore.
- Although ideally PartitionGraph should handle default annotations, we currently do not have this process yet. This PR makes a workaround by adding a temporary pass that removes all default annotations. A TODO was added as a reminder.
**Example Codegen (DNNL) Improvements**
- Support multiple outputs in DNNL codegen.
- Temporary turn off mobilnet unit tests due to constant node issue.
cc @zhiics, @trevor-m, @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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] mbaret commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
mbaret commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611102183
By moving AnnotateRestDefault's functionality into AnnotateTarget, won't that prevent us from being able to run AnnotateTarget multiple times (because it will annotate defaults multiple times)?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-612082540
Let's rebase and bring this in and then we can bring @masahi's PR in 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r406463482
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -74,25 +75,21 @@ void AnnotatedRegionSetNode::AddToRegion(AnnotatedRegion dest, const Expr& expr)
if (src.defined()) {
MergeRegions(src, dest);
} else {
- dest->nodes.insert(expr);
+ dest->nodes_.insert(expr);
}
}
-AnnotatedRegion AnnotatedRegionSetNode::MakeRegion() {
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(std::string target) {
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611100287
@mbaret The flow you mentioned makes sense to me. I think it is really case by case. Supporting annotation of multiple targets doesn't really prevent users from invoking annoate_target multiple times when needed. However, it gives users a globally view of the graph and provides them an opportunity to optimize the scheduling of ops. Annotating graph mutliple times looks reasonable to me, but we should avoid reverting the partitioned graph and repartitioning it as this could be very error-prone.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405904292
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
I understand, but what I said maybe is not clear. I know composite function doesn't jump into OpNode branch, but I mean I only use composite function, so I don't register any op FTVMAnnotateTarget attr for target.xxxx, and for any OpNode，auto fannotate = Op::GetAttr("target." + std::string(target)) will report fail, because I didn't register any FTVMAnnotateTarget attr before. annotate mechanism can handle op and composite function, and the two is independent for each other.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics merged pull request #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
zhiics merged pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] mbaret commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
mbaret commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611084398
To explain the point about QNN a bit better, a concrete example would be our Ethos-N NPU codegen paired up with something like DNNL. The NPU directly supports quantised convolutions and so wants to mark qnn.conv2d as supported. DNNL doesn't supported quantised convolutions but does support normal convolutions, so it wants to see nn.conv2d. Now the TFLite frontend will initially produce a graph containing qnn.conv2d's from a qnn model. If we choose to annotate here, only the Ethos-N codegen will be able to target the graph. If, however, we choose to run QnnCanonicalize, the qnn.conv2d's are lowered to nn.conv2d's but now only DNNL can annotate the graph.
To me the ideal solution here is to be able to run AnnotateTarget multiple times on different lowerings of the graph, because I agree that running the partitioning pipeline multiple times is not a good long term solution (although it probably is a good short term one).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611341598
> composite function itself will not reach the branch, but any other OpNode will reach the branch, and if I only use composite function, I will not register target.xxxx op attr, so OpNode branch will report fail and abort.
Ah I got your point. You're right that check is necessary. I'll add it back. Thanks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
masahi commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611220684
> It may have some conflict with your PR
Yes there will be some conflict, since I refactored dnnl/codegen.cc quite a bit. But it seems we only need to copy paste the change in this PR to `GenerateBody` function in my PR, so I think resolving is easy. For me it is fine if this PR is merged first.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] masahi edited a comment on issue #5277: [BYOC]
Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611220684
> It may have some conflict with your PR
Yes there will be some conflict, since I refactored dnnl/codegen.cc quite a bit. But it seems we only need to copy paste the change in this PR to `GenerateBody` function in my PR, so I think resolving is easy. For me it is fine if this PR is merged first.
Reviewing this PR is a bit difficult for me since I don't know the context of this PR (I don't use either AnnotateTarget or MergeCompilerRegion in my work), and as @mbaret mentioned, there are different kinds of changes (bug fix, improvement etc). I also prefer breaking up into smaller PRs.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405664349
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
Composite functions are FunctionNode so they will not reach to this part but will be at the branch of line 109. That's why I consider this issue is naturally resolved with this change.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405902840
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
I understand, but what I said maybe is not clear. I know composite function doesn't jump into OpNode branch, but I mean I only use composite function, so I don't define any op FTVMAnnotateTarget attr for target.xxxx, and for any OpNode，auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target)) will report fail, because I didn't define any FTVMAnnotateTarget attr before. annotate mechnism can handle op and composite function, and the two is independent for each other.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] mbaret commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
mbaret commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611111173
Has there been consideration of the proposal we made with the graph partitioning RFC (multiple independent annotation passes + a deconflict 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405902840
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
I understand, but what I said maybe is not clear. I know composite function doesn't jump into OpNode branch, but I mean I only use composite function, so I don't define any op FTVMAnnotateTarget attr for target.xxxx, and for any OpNode，auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target)) will report fail, because I didn't define any FTVMAnnotateTarget attr. annotate mechnism can handle op and composite function,
and the two is independent for each other.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] manupa-arm commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-613946486
Sorry for the late comments ( I was out for a week ).
Among the bug fixes (e.g., type propogation in tuple bug) and new functionalities (e.g., supporting a list of targets in AnnotateTarget), this PR had technically moved (and improved) AnnotateRestDefault to AnnotateTarget and moved the removal of defaults to PartitionGraph.
IIRC, the defaults were a requirement of the MergeCompilerRegions pass and thats why they were inside MCR ? I just want to understand the rationale as to why you think they should moved as such and potentially exposing them ("defaults") in the graph that is being passed among passes?
PS : I agree with @zhiics that this pass do not change the flow and agree with the most of the fixes that are brought in alongside the moving of "default" related components, though ideally should have been better if they were seperate PRs.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405902840
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
I understand, but what I said maybe is not clear. I know composite function doesn't jump into OpNode branch, but I mean I only use composite function, so I don't define any op FTVMAnnotateTarget attr for target.xxxx, and for any OpNode，auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target)) will report fail, because I didn't define any FTVMAnnotateTarget attr before. annotate mechnism can handle op and composite function, and the two is independent for each other.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] mbaret commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
mbaret commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-610851491
Thanks for this, a few high level comments first.
- Could we separate some of these changes out into separate PRs? In particular, separating bug fixes from features so we can get them in quickly.
- Am I right to think that the intended flow here is to only run AnnotateTarget once? I'm not sure how we would use it in the case where we have two codegens, one that wants to see the graph with QNN ops and the other than wants the graph without them.
- What's the advantage of this multi-target approach over running the entire partitioning pipeline multiple times?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405303431
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
need add one line: if (!Op::HasAttr("target." + std::string(target))) return false; before auto fannotate
because for composite function, GetAttr maybe failed
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405903872
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
I understand, but what I said maybe is not clear. I know composite function doesn't jump into OpNode branch, but I mean I only use composite function, so I don't define any op FTVMAnnotateTarget attr for target.xxxx, and for any OpNode，auto fannotate = Op::GetAttr("target." + std::string(target)) will report fail, because I didn't define any FTVMAnnotateTarget attr before. annotate mechnism can handle op and composite function, and the two is independent for each other.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405903872
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
I understand, but what I said maybe is not clear. I know composite function doesn't jump into OpNode branch, but I mean I only use composite function, so I don't define any op FTVMAnnotateTarget attr for target.xxxx, and for any OpNode，auto fannotate = Op::GetAttr("target." + std::string(target)) will report fail, because I didn't define any FTVMAnnotateTarget attr before. annotate mechnism can handle op and composite function, and the two is independent for each other.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-614153285
One reason that I exposed default between passes was to have a clear task division so that the flow would be more stable. For example, one bug about composite function reported in #5272 was because AnnotateTarget pass skipped composite functions but AnnotateRestDefault didn't, so the default annotations were mis-inserted and it results in segmentation fault when partitioning the graph. This may happen in the future if still we have more than one places doing similar tasks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611166394
> Has there been consideration of the proposal we made with the graph partitioning RFC (multiple independent annotation passes + a deconflict pass)?
I don't think this PR is against the RFC because this PR doesn't really change the flow you implemented, but just make the annotation pass more general by considering multiple targets. On the other hand, as @zhiics mentioned, you can still run the annotation pass multiple times followed by another deconflict pass if you really have to.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5277:
[BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r406408975
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -74,25 +75,21 @@ void AnnotatedRegionSetNode::AddToRegion(AnnotatedRegion dest, const Expr& expr)
if (src.defined()) {
MergeRegions(src, dest);
} else {
- dest->nodes.insert(expr);
+ dest->nodes_.insert(expr);
}
}
-AnnotatedRegion AnnotatedRegionSetNode::MakeRegion() {
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(std::string target) {
Review comment:
const std::string& 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] windclarion commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r405303431
##########
File path: src/relay/transforms/annotate_target.cc
##########
@@ -19,131 +19,155 @@
/*!
* \file src/relay/transforms/annotate_target.cc
- * \brief Wraps a call with compiler_begin and compiler_end to indicate that
- * the op of this call node will use external compiler.
+ * \brief Wraps an expr with compiler_begin and compiler_end to indicate that
+ * this expr should be handled by the external compiler.
*/
#include <tvm/relay/attrs/annotation.h>
#include <tvm/relay/expr_functor.h>
#include <tvm/relay/op_attr_types.h>
#include <tvm/relay/transform.h>
+#include <tvm/runtime/container.h>
namespace tvm {
namespace relay {
namespace annotate_target {
-// Cache compiler_begin op for equivalence check.
-static const Op& compiler_begin_op = Op::Get("annotation.compiler_begin");
+const PackedFunc* begin_op = runtime::Registry::Get("relay.op.annotation._make.compiler_begin");
+const PackedFunc* end_op = runtime::Registry::Get("relay.op.annotation._make.compiler_end");
// A helper class to insert annotation boundaries for a program region that will
// be handled by a specific compiler.
class AnnotateTargetWrapper : public ExprMutator {
public:
- explicit AnnotateTargetWrapper(const std::string& target) : target_(target) {}
+ explicit AnnotateTargetWrapper(Array<runtime::String> targets) : targets_(std::move(targets)) {}
+
+ /*!
+ * \brief This function annotates a compiler end and a compiler begin to all arguments.
+ *
+ * The compiler end is based on the arg target while the compiler begin is based on the given
+ * target. If target is not given and all arguments are going to the same target, then we will
+ * use that target; otherwise we use default for this op. Note that all arg exprs must be
+ * available in op_expr_to_target before calling this function.
+ *
+ * \param args An array of arguments of the given node.
+ * \param target The target of the current node.
+ * \return A pair of target and annotated argument expressions.
+ */
+ std::pair<std::string, Array<Expr>> AnnotateArgs(const Array<Expr>& args,
+ const std::string& target = "") {
+ std::string ref_target = "";
+ Array<Expr> compiler_ends;
+ for (auto arg : args) {
+ if (op_expr_to_target_.find(arg) != op_expr_to_target_.end()) {
+ std::string arg_target = op_expr_to_target_[arg];
+ compiler_ends.push_back(InsertAnnotation(arg, arg_target, end_op));
+ if (ref_target == "") {
+ ref_target = arg_target;
+ } else if (ref_target != arg_target) {
+ ref_target = "default";
+ }
+ } else {
+ // Input vars.
+ compiler_ends.push_back(arg);
+ }
+ }
+
+ // 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, begin_op));
+ }
- Expr Annotate(const Expr& expr) {
- return InsertEnd(Mutate(expr));
+ return {op_target, compiler_begins};
}
- bool IsSupported(const Expr& expr) {
- if (expr->IsInstance<CallNode>()) {
- Call call = Downcast<Call>(expr);
- auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + target_);
- if (call->op->IsInstance<OpNode>()) {
- Op op = Downcast<Op>(call->op);
- CHECK(op.defined());
- if (fannotate.count(op)) {
- return fannotate[op](call->attrs, call->args);
+ Expr InsertAnnotation(const Expr& expr, const std::string& target, const PackedFunc* ann_op) {
+ Expr new_op = (*ann_op)(expr, target);
+ new_op->checked_type_ = expr->checked_type_;
+ return new_op;
+ }
+
+ Expr VisitExpr_(const CallNode* cn) final {
+ // Supported targets for this node. The order implies the priority.
+ std::vector<std::string> supported_targets;
+
+ // Check which targets this op can be offloaded.
+ if (cn->op->IsInstance<OpNode>()) {
+ // TVM operators: Check target specific op checking function and add to supported_targets
+ // if it is supported.
+ Op op = Downcast<Op>(cn->op);
+ CHECK(op.defined());
+ for (const auto& target : this->targets_) {
+ auto fannotate = Op::GetAttr<FTVMAnnotateTarget>("target." + std::string(target));
Review comment:
maybe need add one line: if (!Op::HasAttr("target." + std::string(target))) return false; before auto fannotate
because for composite function, GetAttr maybe failed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on a change in pull request
#5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r406463527
##########
File path: src/relay/analysis/annotated_region_set.h
##########
@@ -184,11 +193,11 @@ class AnnotatedRegionSetNode : public Object {
void AddToRegion(AnnotatedRegion dest, const Expr& expr);
/*!
- * \brief Make a new region.
+ * \brief Make a new region for a target.
*
* \return The new region.
*/
- AnnotatedRegion MakeRegion();
+ AnnotatedRegion MakeRegion(std::string target);
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on a change in pull request #5277:
[BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#discussion_r406409545
##########
File path: src/relay/analysis/annotated_region_set.h
##########
@@ -184,11 +193,11 @@ class AnnotatedRegionSetNode : public Object {
void AddToRegion(AnnotatedRegion dest, const Expr& expr);
/*!
- * \brief Make a new region.
+ * \brief Make a new region for a target.
*
* \return The new region.
*/
- AnnotatedRegion MakeRegion();
+ AnnotatedRegion MakeRegion(std::string target);
Review comment:
const std::string& 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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611078702
> Thanks for this, a few high level comments first.
>
> * Could we separate some of these changes out into separate PRs? In particular, separating
> bug fixes from features so we can get them in quickly.
It's hard to separate bug fixes to another PR because some fixes were done with refactoring.
> * Am I right to think that the intended flow here is to only run AnnotateTarget once? I'm not sure how we would use it in the case where we have two codegens, one that wants to see the graph with QNN ops and the other than wants the graph without them.
Yes I suppose the flow is intended to run AnnotateTarget only once, but I'm not sure I get your issue. If a user wants to have a QNN model then she will run the QNN pass first anyhow to generate QNN ops in the graph. Under this assumption, if a codegen doesn't want to see QNN ops, it should not have fannotate and AnnotateTarget will annotate QNN ops to other codegens or just default.
> * What's the advantage of this multi-target approach over running the entire partitioning pipeline multiple times?
I think I understand your concern more after this question. You're saying if a user wants to offload a graph to multiple targets then she will run AnnotateTarget->MergeCompilerRegion->PartitionGraph multiple times. IMHO, this is not an ideal flow due to the following reasons:
1. If the flow is designed for offloading only one target, it's hard to consider global optimization over multiple targets in the future. For example, 1st run with TargetA creates 2 subgraphs due to restriction set; 2nd run with TargetB creates 1 subgraph (so 3 subgraphs in total). However, assuming the second subgraph for TargetA can also be offloaded to TargetB, meaning that if we run TargetB first than we will get 2 subgraphs in total.
2. If the flow may take a graph that has been partitioned by another target in advance, all passes in this flow will need to deal with partitioned functions. It's hard to maintain and even harder to integrate global optimization in the above situation.
3. The position of PartitionGraph in the pass manager is meaningful. We guarantee external functions generated by this pass can be processed by following passes, but not others.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611106645
> By moving AnnotateRestDefault's functionality into AnnotateTarget, won't that prevent us from being able to run AnnotateTarget multiple times (because it will annotate defaults multiple times)?
Even the original implementation has the similar issue because it didn't check if an argument is compiler end. I'm making a commit to only modify the annotation attribute for the current target if the annotation has already there.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] zhiics commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
zhiics commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611199100
@masahi please also help review this PR. It may have some conflict with your PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-tvm] comaniac commented on issue #5277: [BYOC] Refine
AnnotateTarget and MergeCompilerRegion Passes
Posted by GitBox <gi...@apache.org>.
comaniac commented on issue #5277: [BYOC] Refine AnnotateTarget and MergeCompilerRegion Passes
URL: https://github.com/apache/incubator-tvm/pull/5277#issuecomment-611344498
@masahi thanks for the suggestion. I've reverted the DNNL part and will file another PR for it.
@windclarion checker added.
----------------------------------------------------------------
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
With regards,
Apache Git Services