Fix laziness closurization bug, add support for ADTs #1789
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yannham
force-pushed
the
fix/laziness-closurization
branch
from
30e385e
to
10ddd1b
Compare
yannham
commented
Jan 29, 2024
jneem
approved these changes
Jan 29, 2024
vkleen
approved these changes
Jan 30, 2024
yannham
force-pushed
the
fix/adt-equality
branch
2 times, most recently
from
2bc3311
to
834f285
Compare
yannham
force-pushed
the
fix/laziness-closurization
branch
from
b1eedd2
to
a93b1c5
Compare
|
🎉 All dependencies have been resolved ! |
This commit fixes an issue that has been introduced by the removal of generated variables, together with the `should_update` optimization, which tries to avoid thunk update that are unnecessary. However, as detailed in the added documentation for should_update, this didn't work well with the removal of generated variables, which would materialize by not sharing inner expressions for variables whose value was already a data structure in weak head normal form (arrays, records or now enum variants). Inner expressions would be recomputed. This commit fixes the issue by adding a condition that datastructure that aren't yet closurized shouldn't eschew thunk update. Doing so, this commit also adds the missing closurization of ADTs, which caused enum variant's arguments to also be potentially re-evaluated many time. Co-authored-by: Viktor Kleen <viktor.kleen@tweag.io>
yannham
force-pushed
the
fix/laziness-closurization
branch
from
a93b1c5
to
7efe6c1
Compare
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #1787
Closes #1788
This PR started as a small follow-up of #1770 adding closurization to ADTs, such that in an expression
'Foo exp(in the ideal final syntax),expis properly shared and not recomputed many times, as for other datastructures (arrays and records).Testing this unveiled #1788 which is related and rather simple to fix, so this PR does both: fixing other datastructure subexpressions being sometimes unduly re-evaluated, and add missing sharing (and thus avoid re-evaluation) for ADTs.