-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[BugFix] Support rewrite_once when the number of callbacks > 1 #14344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
|
What do you think @masahi ? Thank you! |
src/relay/ir/dataflow_matcher.cc
Outdated
| static auto* structural_equal = runtime::Registry::Get("node.StructuralEqual"); | ||
| ICHECK(structural_equal) << "node.StructuralEqual is not registered."; | ||
| // Use the callback until the callback's attribute is rewrite_once=true and has been rewritten | ||
| std::unordered_map<DFPatternCallback, bool, ObjectPtrHash, ObjectPtrEqual> callbacks_map; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
callbacks_map -> done, swapping false and true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take another look. Thank you!
src/relay/ir/dataflow_matcher.cc
Outdated
| bool equal = true; | ||
| static auto* structural_equal = runtime::Registry::Get("node.StructuralEqual"); | ||
| ICHECK(structural_equal) << "node.StructuralEqual is not registered."; | ||
| // Use the callback until the callback's attribute is rewrite_once=true and has been rewritten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Keep track of callbacks that have finished rewriting
429c287 to
d274784
Compare
|
It seems that CI has passed, could you please help me merge this PR? Thanks @masahi ! |
d274784 to
22f86d9
Compare
I don't know whether our repo will automatically rebase when it merges PR, so I just performed a rebase operation myself, do I need to do this every time the main branch is updated? |
|
No you don't have to do that. You now have to wait for another 4h :) |
|
Thanks masahi, I should have consulted before performing the rebase operation :( |
|
CI passed again...I won't do redundant operations this time.hhhh |
The
rewrite_onceof the currentPatternRewriterrewrites once only when the last callback in the callbacks list isrewrite_once. I don't think this is as expected.I think the
rewrite_oncestrategy should be that the callback marked withrewrite_onceis rewritten only once, and the rest of the callbacks are still rewritten multiple times.