-
Notifications
You must be signed in to change notification settings - Fork 97
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
Backend refactor: Move ActorScript name environment further down #430
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
`compile.ml` is mostly split into two parts: * the back-end leaning parts: Heap objects, functions on them etc. * the frontend-leaning parts: Traversing the AST, keeping track of the variable bindings etc. But so far we had one environment for both aspects (e.g. storing names for WebAssembly locals; keeping track of where AS variables are stored). This has bothered me for a long time, and I finally pulled the band-aid: * `E.t` no longer has `local_vars_env`. In fact, `E.t` is only concerned with the bookkeeping needed to prodcue Wasm. Also `env : E.t` now only changes when going into a new _Wasm function_, no longer with every ActorScript scope. For some local `go` function, it no longer needs to be passed around. * There is a new environment `ASEnv.t` which only contains the ActorScript name and label environment. It is defined much futher down, and passed around separately by `compile_exp` etc. Only the `ae : ASEnv.t` changes when entering a scope, so only this needs to be threaded through patterns or declartions etc. This also unbreaks the unholy nested type recursion through `varloc`. I expect that I can simply the `deferred_loc` story now that this is distangeled. In theory we should be able to do `open IR` before the second part, but we are not quite there yet. Bitwise identical Wasm output with this change, according to `./compare-wat`.
Gabor, can I have a review? This is a refactoring, so rather tedious to keep in a feature branch. |
ggreif
approved these changes
May 21, 2019
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 reasonable, let's see how it affects in-flight PRs and branches.
Co-Authored-By: Gabor Greif <[email protected]>
Co-Authored-By: Gabor Greif <[email protected]>
nomeata
added a commit
that referenced
this pull request
May 27, 2019
this is possible since #430. This refactor also opens the door for storing fixed-width numbers and references in Wasm locals directly. No change to the generated wasm code.
nomeata
added a commit
that referenced
this pull request
May 28, 2019
this is possible since #430. This refactor also opens the door for storing fixed-width numbers and references in Wasm locals directly. No change to the generated wasm code.
dfinity-bot
added a commit
that referenced
this pull request
May 24, 2023
## Changelog for candid: Branch: master Commits: [dfinity/candid@f7166f47...9301221a](dfinity/candid@f7166f4...9301221) * [`51c00478`](dfinity/candid@51c0047) spec: support composite query ([dfinity/candid#420](https://togithub.com/dfinity/candid/issues/420)) * [`74869398`](dfinity/candid@7486939) fix recursion test ([dfinity/candid#425](https://togithub.com/dfinity/candid/issues/425)) * [`eb31f888`](dfinity/candid@eb31f88) Use Arc<Type> with `arc_type` feature ([dfinity/candid#407](https://togithub.com/dfinity/candid/issues/407)) * [`b9d6cbff`](dfinity/candid@b9d6cbf) support custom candid path for export_service ([dfinity/candid#421](https://togithub.com/dfinity/candid/issues/421)) * [`fa29c70d`](dfinity/candid@fa29c70) Add config options for Rust binding ([dfinity/candid#422](https://togithub.com/dfinity/candid/issues/422)) * [`f31396d8`](dfinity/candid@f31396d) Bump webpack from 5.24.4 to 5.76.0 in /tools/ui ([dfinity/candid#413](https://togithub.com/dfinity/candid/issues/413)) * [`9301221a`](dfinity/candid@9301221) release ([dfinity/candid#430](https://togithub.com/dfinity/candid/issues/430))
mergify bot
pushed a commit
that referenced
this pull request
May 24, 2023
## Changelog for candid: Branch: master Commits: [dfinity/candid@f7166f47...9301221a](dfinity/candid@f7166f4...9301221) * [`51c00478`](dfinity/candid@51c0047) spec: support composite query ([dfinity/candid#420](https://togithub.com/dfinity/candid/issues/420)) * [`74869398`](dfinity/candid@7486939) fix recursion test ([dfinity/candid#425](https://togithub.com/dfinity/candid/issues/425)) * [`eb31f888`](dfinity/candid@eb31f88) Use Arc<Type> with `arc_type` feature ([dfinity/candid#407](https://togithub.com/dfinity/candid/issues/407)) * [`b9d6cbff`](dfinity/candid@b9d6cbf) support custom candid path for export_service ([dfinity/candid#421](https://togithub.com/dfinity/candid/issues/421)) * [`fa29c70d`](dfinity/candid@fa29c70) Add config options for Rust binding ([dfinity/candid#422](https://togithub.com/dfinity/candid/issues/422)) * [`f31396d8`](dfinity/candid@f31396d) Bump webpack from 5.24.4 to 5.76.0 in /tools/ui ([dfinity/candid#413](https://togithub.com/dfinity/candid/issues/413)) * [`9301221a`](dfinity/candid@9301221) release ([dfinity/candid#430](https://togithub.com/dfinity/candid/issues/430))
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.
compile.ml
is mostly split into two parts:variable bindings etc.
But so far we had one environment for both aspects (e.g. storing names
for WebAssembly locals; keeping track of where AS variables are stored).
This has bothered me for a long time, and I finally pulled the band-aid:
E.t
no longer haslocal_vars_env
. In fact,E.t
is only concernedwith the bookkeeping needed to prodcue Wasm.
Also
env : E.t
now only changes when going into a new Wasm function,no longer with every ActorScript scope. For some local
go
function,it no longer needs to be passed around.
There is a new environment
ASEnv.t
which only contains theActorScript name and label environment. It is defined much futher
down, and passed around separately by
compile_exp
etc.Only the
ae : ASEnv.t
changes when entering a scope, so only thisneeds to be threaded through patterns or declarations etc.
This also unbreaks the unholy nested type recursion through
varloc
. Iexpect that I can simply the
deferred_loc
story now that this isdistangeled.
In theory we should be able to do
open IR
before the second part, butwe are not quite there yet.
Bitwise identical Wasm output with this change, according to
./compare-wat
.