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 2022/04/12 09:40:19 UTC
[GitHub] [tvm] masahi opened a new pull request, #10975: [Metaschedule] Make custom schedule_rule registration optional
masahi opened a new pull request, #10975:
URL: https://github.com/apache/tvm/pull/10975
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] masahi commented on a diff in pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #10975:
URL: https://github.com/apache/tvm/pull/10975#discussion_r849055781
##########
src/meta_schedule/space_generator/post_order_apply.cc:
##########
@@ -136,19 +136,31 @@ class PostOrderApplyNode : public SpaceGeneratorNode {
stack.emplace_back(sch, blocks);
continue;
}
+
Optional<String> ann = tir::GetAnn<String>(sch->GetSRef(block_rv), "schedule_rule");
- if (ann.defined() == sch_rule.defined() || (ann.defined() && ann.value() == "None")) {
+ bool has_schedule_rule = ann.defined() && runtime::Registry::Get(ann.value()) != nullptr;
+
+ if (ann.defined() && !has_schedule_rule) {
+ LOG(WARNING) << "Custom schedule rule not found, ignoring schedule_rule annotation: "
+ << ann.value();
+ }
+
+ if ((has_schedule_rule && sch_rule.defined()) ||
+ (!has_schedule_rule && !sch_rule.defined()) ||
Review Comment:
`ann.defined() == sch_rule.defined()` condition is replaced with this condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 merged pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged PR #10975:
URL: https://github.com/apache/tvm/pull/10975
--
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] junrushao1994 commented on pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on PR #10975:
URL: https://github.com/apache/tvm/pull/10975#issuecomment-1097298098
CC: @zxybazh
--
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] masahi commented on a diff in pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #10975:
URL: https://github.com/apache/tvm/pull/10975#discussion_r849101798
##########
src/meta_schedule/space_generator/post_order_apply.cc:
##########
@@ -136,19 +136,31 @@ class PostOrderApplyNode : public SpaceGeneratorNode {
stack.emplace_back(sch, blocks);
continue;
}
+
Optional<String> ann = tir::GetAnn<String>(sch->GetSRef(block_rv), "schedule_rule");
- if (ann.defined() == sch_rule.defined() || (ann.defined() && ann.value() == "None")) {
+ bool has_schedule_rule = ann.defined() && runtime::Registry::Get(ann.value()) != nullptr;
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
masahi commented on PR #10975:
URL: https://github.com/apache/tvm/pull/10975#issuecomment-1097436077
Sorry the last commit makes the CI pass but it broke my auto-tensorization experiment. Fixing.
--
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] zxybazh commented on pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
zxybazh commented on PR #10975:
URL: https://github.com/apache/tvm/pull/10975#issuecomment-1098335681
The PR looks good to me, would you take another look @junrushao1994 ?
--
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] zxybazh commented on a diff in pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
zxybazh commented on code in PR #10975:
URL: https://github.com/apache/tvm/pull/10975#discussion_r849100935
##########
tests/python/unittest/test_meta_schedule_post_order_apply.py:
##########
@@ -371,8 +371,8 @@ def test_meta_schedule_custom_search_space():
)
post_order_apply = PostOrderApply()
post_order_apply.initialize_with_tune_context(context)
- with pytest.raises(ValueError, match="Custom schedule rule not found"):
- post_order_apply.generate_design_space(mod)
+
+ post_order_apply.generate_design_space(mod)
Review Comment:
I see, so it will raise a warning first and then do the acutal work later in line 386. That make sense.
--
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] masahi commented on a diff in pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #10975:
URL: https://github.com/apache/tvm/pull/10975#discussion_r849099253
##########
tests/python/unittest/test_meta_schedule_post_order_apply.py:
##########
@@ -371,8 +371,8 @@ def test_meta_schedule_custom_search_space():
)
post_order_apply = PostOrderApply()
post_order_apply.initialize_with_tune_context(context)
- with pytest.raises(ValueError, match="Custom schedule rule not found"):
- post_order_apply.generate_design_space(mod)
+
+ post_order_apply.generate_design_space(mod)
Review Comment:
L375 is the missing custom schedule rule scenario. Previously it would raise error (caught by `with pytest.raises`).
--
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] zxybazh commented on a diff in pull request #10975: [Metaschedule] Make custom schedule_rule registration optional
Posted by GitBox <gi...@apache.org>.
zxybazh commented on code in PR #10975:
URL: https://github.com/apache/tvm/pull/10975#discussion_r849092781
##########
src/meta_schedule/space_generator/post_order_apply.cc:
##########
@@ -136,19 +136,31 @@ class PostOrderApplyNode : public SpaceGeneratorNode {
stack.emplace_back(sch, blocks);
continue;
}
+
Optional<String> ann = tir::GetAnn<String>(sch->GetSRef(block_rv), "schedule_rule");
- if (ann.defined() == sch_rule.defined() || (ann.defined() && ann.value() == "None")) {
+ bool has_schedule_rule = ann.defined() && runtime::Registry::Get(ann.value()) != nullptr;
Review Comment:
Shall we store the value of `runtime::Registry::Get(ann.value())` so that we don't need to do that again later?
##########
src/meta_schedule/space_generator/post_order_apply.cc:
##########
@@ -136,19 +136,31 @@ class PostOrderApplyNode : public SpaceGeneratorNode {
stack.emplace_back(sch, blocks);
continue;
}
+
Optional<String> ann = tir::GetAnn<String>(sch->GetSRef(block_rv), "schedule_rule");
- if (ann.defined() == sch_rule.defined() || (ann.defined() && ann.value() == "None")) {
+ bool has_schedule_rule = ann.defined() && runtime::Registry::Get(ann.value()) != nullptr;
+
+ if (ann.defined() && !has_schedule_rule) {
+ LOG(WARNING) << "Custom schedule rule not found, ignoring schedule_rule annotation: "
+ << ann.value();
+ }
+
+ if ((has_schedule_rule && sch_rule.defined()) ||
+ (!has_schedule_rule && !sch_rule.defined()) ||
Review Comment:
Looks good to me.
##########
tests/python/unittest/test_meta_schedule_post_order_apply.py:
##########
@@ -371,8 +371,8 @@ def test_meta_schedule_custom_search_space():
)
post_order_apply = PostOrderApply()
post_order_apply.initialize_with_tune_context(context)
- with pytest.raises(ValueError, match="Custom schedule rule not found"):
- post_order_apply.generate_design_space(mod)
+
+ post_order_apply.generate_design_space(mod)
Review Comment:
Would you please keep another test for missing custom schedule rule scenario?
##########
src/meta_schedule/space_generator/post_order_apply.cc:
##########
@@ -136,19 +136,31 @@ class PostOrderApplyNode : public SpaceGeneratorNode {
stack.emplace_back(sch, blocks);
continue;
}
+
Optional<String> ann = tir::GetAnn<String>(sch->GetSRef(block_rv), "schedule_rule");
- if (ann.defined() == sch_rule.defined() || (ann.defined() && ann.value() == "None")) {
+ bool has_schedule_rule = ann.defined() && runtime::Registry::Get(ann.value()) != nullptr;
+
+ if (ann.defined() && !has_schedule_rule) {
+ LOG(WARNING) << "Custom schedule rule not found, ignoring schedule_rule annotation: "
+ << ann.value();
+ }
+
+ if ((has_schedule_rule && sch_rule.defined()) ||
+ (!has_schedule_rule && !sch_rule.defined()) ||
+ (ann.defined() && ann.value() == "None")) {
stack.emplace_back(sch, blocks);
continue;
}
+
Array<tir::Schedule> applied{nullptr};
if (sch_rule.defined()) {
applied = sch_rule.value()->Apply(sch, /*block=*/block_rv);
- } else {
+ } else if (has_schedule_rule) {
Review Comment:
This acutally definitely hold true because we have the check in line 149.
--
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