Skip to content
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

fix: move not resetting source location #1293

Merged
merged 6 commits into from
May 2, 2024

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Apr 30, 2024

Summary

Use a MIR pass to lower the move and wasMoved magic calls into
assignments, fixing move not always resetting the source location for
both the VM and JS backends. The C backend was not affected.

Details

A x = move(y) call is now lowered into the following MIR:

x = move y
y = default()

and a wasMoved(x) call into:

x = default()

which is equivalent in behaviour to the code previously produced by
cgen -- using a shared MIR pass for the lowering ensures that the
behaviour is consistent across all backends.

The implementation of the two magics is removed from each code
generator, and a test for making sure move works correctly is added.

Previous behaviour

For the VM backend, the source location was never reset on move,
while for the JS backend, the source location was reset with
genericReset, which left record-like locations empty (reading the
fields then yield undefined) and ignores primitive types like int or
float.

Instead of in the code generator(s), lower the magics with a combined
MIR pass. This also fixes `move` calls not resetting the source
location when using the VM backend (`vmgen` implemented the magic
improperly). `jsgen` had a similar bug, where locations of primitive
types were not reset properly -- this is fixed too.
Handling the `mMove` and `mWasMoved` magics is obsolete.
@zerbina zerbina added bug Something isn't working compiler/backend Related to backend system of the compiler labels Apr 30, 2024
@zerbina zerbina added this to the MIR phase milestone Apr 30, 2024
@zerbina
Copy link
Collaborator Author

zerbina commented Apr 30, 2024

Scanning the body of every procedure for magics has overhead, and it'd be better to only run the pass if the relevant magics are actually used within the body.

My plan is store a set[TMagic] in MirBody, for keeping track of all magics currently present in the body. A pass for lowering, say, mWasMoved then only has to run when the magic is present in the set.


As for mMove, it ideally should be turned into def _1 = move x; wasMoved(name x) before the move analyzer runs. At the moment, the move analyzer doesn't know that a move(x) call actually moves the value out of the location, preventing some optimizations.

For example, for:

var x = @[...]
var y = move(x)

the compiler currently injects a destructor for both x and y, even though x is guaranteed to not require destruction (the value was moved on all paths).

Checking whether `extract` reset the value was disabled for the VM,
since `move` did previously not reset the source location.
This was a change that unintentionally made its way into the earlier
commit.
@saem
Copy link
Collaborator

saem commented May 2, 2024

/merge

Copy link

github-actions bot commented May 2, 2024

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • more progress towards simplifying code generation
  • a separate fix is required for the tarcmisc.nim test failure

@chore-runner chore-runner bot added this pull request to the merge queue May 2, 2024
Merged via the queue into nim-works:devel with commit 6e8ab5a May 2, 2024
31 checks passed
@zerbina zerbina deleted the lower-move-and-wasmoved branch May 6, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/backend Related to backend system of the compiler
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants