-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Legalize - Use Non-recursive Rewriter. #5296
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
|
@mbrookhart can you help to review the PR |
mbrookhart
left a comment
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.
LGTM. Thanks!
src/relay/transforms/legalize.cc
Outdated
| Expr Rewrite_(const CallNode* call_node, const Expr& post) override { | ||
| // Get the new_call node without any changes to current call node. | ||
| Expr new_e = ExprMutator::VisitExpr_(call_node); | ||
| Expr new_e = post; |
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.
Can we just rename post to new_e and remove this line?
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.
I could not directly rename, as new_e was supposed to be the new expr and post is a cont. But, I have cleaned up the code, so that new_e is not needed anymore.
tmoreau89
left a comment
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.
great to see the non-recursive rewriter being used!
|
@LiangHao151941 I think you could benefit this in your pr #4828 |
* Legalize - Use Non-recursive Rewriter. * Cleanup.
* Legalize - Use Non-recursive Rewriter. * Cleanup.
* Legalize - Use Non-recursive Rewriter. * Cleanup.
One of my pre-quantized model parsing was seeing segfault because of stack overflow. Switching to non-recursive mutation resolves the issue, and I can compile the model successfully. Thanks for this work!
@mbrookhart @tqchen