-
-
Notifications
You must be signed in to change notification settings - Fork 670
Experiment: What if locals are never free'd #1260
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
(local $8 i32) | ||
(local $9 i32) | ||
(local $10 i32) | ||
(local $11 i32) | ||
(local $12 i32) | ||
(local $13 i32) | ||
(local $14 i32) | ||
(local $15 i32) | ||
(local $16 i32) | ||
(local $17 i32) | ||
(local $18 i32) | ||
(local $19 i32) | ||
(local $20 i32) | ||
(local $21 i32) | ||
(local $22 i32) | ||
(local $23 i32) | ||
(local $24 i32) |
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.
Typically looks like this in untouched output, while optimized output only shuffles around some locals, if anything at all.
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.
It's interesting that we can't optimize it away 🤔 seems like we should be able to see that the last use of a local occurs before the first use of another local and then make them into the same local
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.
Note that this is untouched output without any optimizations. Collapsing these is easy for Binaryen, which does this under any opt level. What I'm interested in here is how much of a problem long local lists actually are, since removing the deduplication we have in the compiler right now would make certain things easier to reason about.
Regarding the failing tests, the most straight-forward option would be to unroll the first loop iteration if incompatible local states are detected. In most cases, this will not be necessary, while in the other cases it guarantees that local states are exactly what subsequent code expects, eliminating edge cases of recompilation by actually compiling twice. The cost we'd pay there is that the body of the loop is essentially emitted twice in uncommon cases, doubling code size of the loop, so a follow-up might be to look into optimizing such patterns in Binaryen where possible. |
Keeping this PR open as part of 2020 vacuum since I'm planning to look into this further. In general it appears good to do as it simplifies some parts of the compiler a lot, making it easier to contribute. |
In #1240 it came up that the way we free temporary locals for reuse during compilation is both unexpected for developers new to the codebase as well as hard to get right when passing compiled expressions that might or might not (re)use a temporary local down to other code paths.
As such, this is an experiment what'd happen if every top-level, scoped and temporary local would be unique, making this process more convenient and less error-prone at the cost of emitting larger untouched functions. Note that the
std/set
andstd/map
tests don't compile this way currently due to unification of local states in loops, needing a new mechanism if we decide to go this route.