-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Legalize - Use Non-recursive Rewriter. #5296
Conversation
@mbrookhart can you help to review the PR |
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
// 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.
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