bug: NewObjE initialisation short-circuiting with mutable variables is changing behaviour
#4623
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
in the presence of side-effects
crusso
approved these changes
Jul 19, 2024
mergify bot
pushed a commit
that referenced
this pull request
Aug 2, 2024
…and adapt `Ir.VarE` (#4637) By making reads from mutable variables distinguishable, we restore the referential transparency of `VarE (Const, id)`. This was the reason why #4611 failed (and had to be reverted in #4623). This opens up a bunch of optimisation possibilities - `VarE` path compression - dead `LetD` elimination - `BlockE`/`NewObjE` simplification - nested `BlockE` merging (given the defs are disjoint and outer uses don't reference inner ones; use `freevars.ml`) - etc. In the long run this may allow us to extend the IR effect system with {`Load`, `Store`} effects, and thus pave the way for transactional memory. We now have a note on `Syntax.VarE`'s identifier, being immutable by default. When performing type checking, this field can change to signify a mutable variable source after looking up the identifier in the environment. Then desugaring uses mutability info to create either `IR.VarE (Const, _)` (for immutable) or a`IR.VarE (Var, _) (for mutable). The IR type checker then asserts that the right kind of access is used. The Wasm output should be unchanged. _NOTE_: with this PR `git revert 13d9d57` can be done and it passes. So it seems to work 😀
mergify bot
pushed a commit
that referenced
this pull request
Aug 8, 2024
…jE` (#4645) This reapplies #4611 without changes (that were previously) necessary to the interpreter and IR checker. PR #4611 had to be backed out due to changed behaviour w.r.t. mutable variables: #4623. ### From #4611: `NewObjE` wants variable names for field values. When the expression is _already a variable_ then just reuse its name! Also, if we managed to eliminate all necessary variable (re-)bindings, then the enclosing new scope (`BlockE`) is redundant. By generating less IR we speed up compilation, and potentially create opportunities for further simplifications. Another positive is that reading the IR (`-dl` flag) becomes less convoluted. _Note_: Since the IR recently has been taught to track whether the read is from an immutable or from a mutable variable, we can now observe renaming of immutable variables and eliminate only those.
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.
This PR extends testcase
run-drun/4611.moto demonstrate evaluation order problem in the presence of side-effects.For reading mutable variables it is relevant when the read happens. If there is an assignment to the mutable cell in-between two reads (also via some call or aliased cell) then the read result will differ.
In #4611 we change the point in time when a mutable variable was read, so a test can be fabricated to create a discrepancy between the reference interpreter and the other executions.
This PR backs out the flawed logic of #4611, but keep the test.
To properly fix such an optimisation one needs a hint whether the variable we read from is mutable. If so, the variable rebinding needs to be kept.
One could (contextually) lower mutable reads to a primitive (see C compiler's
volatilelowering) or haveVarEwith an additional type field (ref typ) that gets filled by the type checker.