[WIP] Various bugs in passes #6906
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
I hope we could come up with a way to avoid this. This would hurt performance on hummingbird workload and possibly other models.
There was a problem hiding this comment.
For example, maybe we can use annotation.stop_fusion when we encounter take + dynamic, solving this problem at the frontend.
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this comment.
@mbrookhart Can you add TODO(masahi) there?
There was a problem hiding this comment.
@masahi want to request changes so this doesn't merge while we chat about it?
There was a problem hiding this comment.
sure done. But I think it is good to go after adding a comment on why we make this change temporary
There was a problem hiding this 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.
| @@ -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) { | |||
There was a problem hiding this comment.
cc @kevinthesun This might be of interest to you
mbrookhart
force-pushed
the
mbrookhart/pass_bugs
branch
from
ce42b5a
to
1f9d81f
Compare
|
@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. |
|
@mbrookhart maybe you can summarize what the issue is with I also want to know why this could lead to infinite loop inside |
|
I've been working on other things, sorry for the delay on this. take with dynamic input shapes is a bit of an issue because take folds the input shape into the compute: tvm/include/tvm/topi/transform.h Line 760 in c58b20b If the input tensor to take is fused into other ops, that input shape maybe removed in the fusion, and then we'll have a reference to an input dimension that no longer exists, so the fused function will fail to compile. As @masahi and I discussed above, there are two main ways to get around this: We can write a pass that breaks the fusion at an earlier step, or we can remove the A third option I've thought of in the meantime is to do the raising of the logic in a legalization pass only for dynamic input shapes. This would allow us to remove the dynamic issue but keep the current code everywhere else. Any thoughts on the best way to proceed? |
This adds regression tests for a number of bugs uncovered while testing ONNX Object Detection models, and adds regression tests for the fixes
@tqchen