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 2021/07/13 21:28:28 UTC
[GitHub] [tvm] zxy844288792 opened a new pull request #8464: [BYOC] add multi functions support in partition pass
zxy844288792 opened a new pull request #8464:
URL: https://github.com/apache/tvm/pull/8464
If a module contains multiple functions, the partition pass fail with this kind of message: ```Check failed: (!module_->ContainGlobalVar(fname)) is false: Global function ccompiler_0 already exists```
The reason of that is due to for each function in the module we maintain a AnnotatedRegion object and reset the region_id which region_id is used to form the subfunction name. Here introduces the new naming which will also use the function name to form the final subfunction name.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669156343
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -76,17 +76,21 @@ void AnnotatedRegionSetNode::AddToRegion(AnnotatedRegion dest, const Expr& expr)
}
}
-AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& target) {
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& func_name,
+ const std::string& target) {
auto ret = regions_.emplace(AnnotatedRegion());
(*ret.first)->id_ = region_id_++;
(*ret.first)->target_ = target;
+ (*ret.first)->func_name_ = func_name;
return *ret.first;
}
class AnnotatedRegionSet::Creator : protected MixedModeVisitor {
public:
Creator(const Op& region_begin_op, const Op& region_end_op)
: begin_op_(region_begin_op), end_op_(region_end_op) {}
+ Creator(const Op& region_begin_op, const Op& region_end_op, const std::string& func_name)
+ : begin_op_(region_begin_op), end_op_(region_end_op), func_name(func_name) {}
Review comment:
Hmm I see. So in the case of merge compiler region we don't really care its name. How about keeping one constructor and assign a default value to func_name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Leo-arm commented on pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
Leo-arm commented on pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#issuecomment-881550254
The hashes need to be updated. I got most of the way through that; I will finish this on Monday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxy844288792 commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669168686
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -76,17 +76,21 @@ void AnnotatedRegionSetNode::AddToRegion(AnnotatedRegion dest, const Expr& expr)
}
}
-AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& target) {
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& func_name,
+ const std::string& target) {
auto ret = regions_.emplace(AnnotatedRegion());
(*ret.first)->id_ = region_id_++;
(*ret.first)->target_ = target;
+ (*ret.first)->func_name_ = func_name;
return *ret.first;
}
class AnnotatedRegionSet::Creator : protected MixedModeVisitor {
public:
Creator(const Op& region_begin_op, const Op& region_end_op)
: begin_op_(region_begin_op), end_op_(region_end_op) {}
+ Creator(const Op& region_begin_op, const Op& region_end_op, const std::string& func_name)
+ : begin_op_(region_begin_op), end_op_(region_end_op), func_name(func_name) {}
Review comment:
agree, just addressed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxy844288792 commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669168791
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -301,7 +302,7 @@ class Partitioner : public MixedModeMutator {
}
std::string target = end_node->attrs.as<CompilerAttrs>()->compiler;
- std::string name = target + "_" + std::to_string(region->GetID());
+ std::string name = target + "_" + std::to_string(region->GetID()) + region->GetName();
Review comment:
Agree, the only reason I put in that way is due to when name is "dafault" then GetName() method will return an empty string and I was afraid it would cause some issues if i changed that. I agree this is more clear and will test it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxy844288792 commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669155020
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -213,12 +217,19 @@ class AnnotatedRegionSet::Creator : protected MixedModeVisitor {
const Op begin_op_;
/*! \brief Region 'end' annotation operator. */
const Op end_op_;
+ /*! \brief The function name.*/
+ const std::string func_name;
Review comment:
will address 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669130328
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -213,12 +217,19 @@ class AnnotatedRegionSet::Creator : protected MixedModeVisitor {
const Op begin_op_;
/*! \brief Region 'end' annotation operator. */
const Op end_op_;
+ /*! \brief The function name.*/
+ const std::string func_name;
Review comment:
```suggestion
/*! \brief The unique function name that is used to be the name of this region set. */
const std::string func_name_;
```
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -301,7 +302,7 @@ class Partitioner : public MixedModeMutator {
}
std::string target = end_node->attrs.as<CompilerAttrs>()->compiler;
- std::string name = target + "_" + std::to_string(region->GetID());
+ std::string name = target + "_" + std::to_string(region->GetID()) + region->GetName();
Review comment:
Let's make it clearer: target - region (function) - index.
```suggestion
std::string name = target + "_" + region->GetName() + "_" + std::to_string(region->GetID());
```
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -76,17 +76,21 @@ void AnnotatedRegionSetNode::AddToRegion(AnnotatedRegion dest, const Expr& expr)
}
}
-AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& target) {
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& func_name,
+ const std::string& target) {
auto ret = regions_.emplace(AnnotatedRegion());
(*ret.first)->id_ = region_id_++;
(*ret.first)->target_ = target;
+ (*ret.first)->func_name_ = func_name;
return *ret.first;
}
class AnnotatedRegionSet::Creator : protected MixedModeVisitor {
public:
Creator(const Op& region_begin_op, const Op& region_end_op)
: begin_op_(region_begin_op), end_op_(region_end_op) {}
+ Creator(const Op& region_begin_op, const Op& region_end_op, const std::string& func_name)
+ : begin_op_(region_begin_op), end_op_(region_end_op), func_name(func_name) {}
Review comment:
Is there any reason to keep both constructors?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Leo-arm commented on pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
Leo-arm commented on pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#issuecomment-881293661
I'll have a look. If we can hold off momentary I might be able to fix this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxy844288792 commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669154906
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -301,7 +302,7 @@ class Partitioner : public MixedModeMutator {
}
std::string target = end_node->attrs.as<CompilerAttrs>()->compiler;
- std::string name = target + "_" + std::to_string(region->GetID());
+ std::string name = target + "_" + std::to_string(region->GetID()) + region->GetName();
Review comment:
Agree, the only reason I put in that way is due to when name is "dafault" then GetName() method will return an empty string and I was afraid it would cause some issues if i changed that. I agree this is more clear and will test it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
leandron commented on pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#issuecomment-880223514
> LGTM
> @leandron the tests in Ethos-N are failed due to the change of function names by this PR. I asked @zxy844288792 to disable them to unblock the CI. Please feel free to re-enable them later on.
Thanks for letting me know. We’ll have a look tomorrow. 👍🏻
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669175601
##########
File path: src/relay/transforms/partition_graph.cc
##########
@@ -116,9 +116,11 @@ class Partitioner : public MixedModeMutator {
for (auto f : module->functions) {
GlobalVar f_var = f.first;
BaseFunc f_func = f.second;
+ std::string f_name = f_var.as<GlobalVarNode>()->name_hint;
Review comment:
Better to have a set here to make sure the function names are unqiue. Something like:
```
while (func_names.find(f_name) != func_names.end()) {
f_name += "_a";
}
func_names.insert(f_name);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zxy844288792 commented on a change in pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
zxy844288792 commented on a change in pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#discussion_r669153705
##########
File path: src/relay/analysis/annotated_region_set.cc
##########
@@ -76,17 +76,21 @@ void AnnotatedRegionSetNode::AddToRegion(AnnotatedRegion dest, const Expr& expr)
}
}
-AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& target) {
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion(const std::string& func_name,
+ const std::string& target) {
auto ret = regions_.emplace(AnnotatedRegion());
(*ret.first)->id_ = region_id_++;
(*ret.first)->target_ = target;
+ (*ret.first)->func_name_ = func_name;
return *ret.first;
}
class AnnotatedRegionSet::Creator : protected MixedModeVisitor {
public:
Creator(const Op& region_begin_op, const Op& region_end_op)
: begin_op_(region_begin_op), end_op_(region_end_op) {}
+ Creator(const Op& region_begin_op, const Op& region_end_op, const std::string& func_name)
+ : begin_op_(region_begin_op), end_op_(region_end_op), func_name(func_name) {}
Review comment:
since it is being used here: https://github.com/apache/tvm/blob/73b38e894fcf4263eeb3978c85c2786218acaff8/src/relay/transforms/merge_compiler_regions.cc#L166
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac merged pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
comaniac merged pull request #8464:
URL: https://github.com/apache/tvm/pull/8464
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #8464: [BYOC] add multi functions support in partition pass
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #8464:
URL: https://github.com/apache/tvm/pull/8464#issuecomment-881580231
Thanks @zxy844288792.
@Leo-arm Right. The hash has to be updated when a PR changes the graph (partitioned function names in this case). It's actually not a good practice as people need to call out ARM folks every time. It would be better if we can improve these tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org