-
Notifications
You must be signed in to change notification settings - Fork 36
Add Constant Propagation pass after MIR passes #439
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
Add Constant Propagation pass after MIR passes #439
Conversation
40a3c8e to
31ceb57
Compare
Node and Owner singletons are now set at initialization time, so now we can lift the invariant that the inner Op/Root needed to be reachable to update the enum variant.
31ceb57 to
b76d029
Compare
bobbinth
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.
Looks good! Thank you! I left a few questions/comments inline.
air-script/tests/list_comprehension/list_comprehension_nested.rs
Outdated
Show resolved
Hide resolved
bobbinth
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.
Looks good! Thank you! I left a few small comments inline. After these are addressed, we should be good to merge.
One general question: what's the main reason to do constant propagation in MIR rather than in the AlgebraicGraph? It seems like doing it in the AlgebraicGraph could be a bit more straight-forward?
Good point! Initially I just put it there as it was a regression introduced by MIR (expressions previously handled in AST's ConstantPropagation were no longer handled), but it would have been easier in the Air. |
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.
Looks good! Thank you!
Good point! Initially I just put it there as it was a regression introduced by MIR (expressions previously handled in AST's ConstantPropagation were no longer handled), but it would have been easier in the Air.
But now I think the main reason to keep it here is to ensure that computed indices (#444) are folded before the end of MIR.
This ensures that we do not have to handle vectors in the AIR (or during lowering of MIR to AIR).
Makes sense! Let's still create an issue to investigate moving constant folding into AIR, but maybe keeping a simpler pass for index folding here (not sure it would make sense - but we can discuss that in the issue).
Perfect, here is the issue: #446 |
This PR closes #427
Tasks: