You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/11/12 18:23:40 UTC
[GitHub] [incubator-tvm] mbrookhart opened a new pull request #6906: Various bugs in passes
mbrookhart opened a new pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906
This adds regression tests for a number of bugs uncovered while testing ONNX Object Detection models, and adds regression tests for the fixes
@tqchen
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6906: [WIP] Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#issuecomment-726427732
@mbrookhart maybe you can summarize what the issue is with `take` + dynamic (for me + others to comment)? I haven't looked into `take` topi compute in detail, so I haven't understood what's wrong with it or what do you mean by "normalization".
I also want to know why this could lead to infinite loop inside `rewrite_simplify` (visiting the same node over and over again) @tqchen
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#issuecomment-726416125
@mbrookhart perhaps we can separate out the other things (fold scale axis and bound) into separate PRs so we can merge them in first. It would also makes the PR itself more informative.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522419567
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
I hope we could come up with a way to avoid this. This would hurt performance on hummingbird workload and possibly other models.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbrookhart commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522516014
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
@masahi want to request changes so this doesn't merge while we chat about 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522517058
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
sure done. But I think it is good to go after adding a comment on why we make this change temporary
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbrookhart commented on pull request #6906: [WIP] Various bugs in passes
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#issuecomment-726416707
@tqchen @masahi That sounds good, I'll split the other parts off into a new PR, and we can wait on `take` until we have a more robust solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522514891
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
Yes a new pass that inserts `stop_fusion` sounds good. I can work on this. We can make `take` opaque for now until I have that new pass ready (or `take` compute being fixed).
Anyway I think graph runtime shouldn't be affected by the issue of `take` + dynamic, so `take` should be injective eventually.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbrookhart commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522500934
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
Good thing I have a typo in CI.
I'm not sure I see a clean way to do this in the frontends, it demands we already have infer_type run to check for dynamic inputs.
Maybe we write a pass that selectively stops fusion on certain ops under certain conditions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522514891
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
Yes a new pass that inserts `stop_fusion` sounds good. I can work on this. We can make `take` opaque for now until I have that new pass ready.
Anyway I think graph runtime shouldn't be affected by the issue of `take` + dynamic, so `take` should be injective eventually.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522513051
##########
File path: src/relay/transforms/fold_scale_axis.cc
##########
@@ -243,7 +243,18 @@ class ForwardPrep : private ExprVisitor {
}
}
// Visitor pattern override.
- void VisitExpr_(const LetNode* call) { LOG(FATAL) << "FoldScaleAxis only accept dataflow-form"; }
+ void VisitExpr_(const LetNode* op) {
Review comment:
cc @kevinthesun This might be of interest to you
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] mbrookhart commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522517222
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
I think the other option is to edit the relay take function that creates the op. We could remove the normalization that causes this problem from the take kernel in topi, and do in it relay with select/shape_of, but that might end up causing some performance degradation, it's hard to predict.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522515694
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
@mbrookhart Can you add `TODO(masahi)` 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
[GitHub] [incubator-tvm] masahi edited a comment on pull request #6906: [WIP] Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi edited a comment on pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#issuecomment-726427732
@mbrookhart maybe you can summarize what the issue is with `take` + dynamic (for me + others to comment)? I haven't looked into `take` topi compute in detail, so I haven't understood what's wrong with it.
I also want to know why this could lead to infinite loop inside `rewrite_simplify` (visiting the same node over and over again) @tqchen
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on a change in pull request #6906: Various bugs in passes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #6906:
URL: https://github.com/apache/incubator-tvm/pull/6906#discussion_r522420882
##########
File path: src/relay/op/tensor/transform.cc
##########
@@ -1076,7 +1076,7 @@ Examples::
.set_support_level(3)
.add_type_rel("Take", TakeRel)
.set_attr<FTVMCompute>("FTVMCompute", TakeCompute)
- .set_attr<TOpPattern>("TOpPattern", kInjective);
+ .set_attr<TOpPattern>("TOpPattern", kOpaque);
Review comment:
For example, maybe we can use `annotation.stop_fusion` when we encounter take + dynamic, solving this problem at the frontend.
----------------------------------------------------------------
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