Add an option for the PostEmscripten pass to set the sbrk ptr location (which is called DYNAMICTOP_PTR in emscripten)#2325
Merged
Conversation
…n (DYNAMICTOP_PTR in emscripten). By setting this at compile time we can reduce code size and avoid importing it at runtime.
jgravelle-google
approved these changes
Sep 4, 2019
| } | ||
|
|
||
| // Optimize calls | ||
| OptimizeCalls().run(runner, module); |
Contributor
There was a problem hiding this comment.
This feels awkward, but I'm not sure if there's a better way to replace sbrk keeping the order before this
Member
Author
There was a problem hiding this comment.
Yeah. I think some of the fastcomp-specific parts can be removed when we remove fastcomp, and then this pass can get simpler again.
kripken
added a commit
that referenced
this pull request
Sep 6, 2019
Currently emscripten links the wasm, then links the JS, then computes the final static allocations and in particular the location of the sbrk ptr (i.e. the location in memory of the brk location). Emscripten then imports that into the asm.js or wasm as env.DYNAMICTOP_PTR. However, this didn't work in the wasm backend where we didn't have support for importing globals from JS, so we implement sbrk in JS. I am proposing that we change this model to allow us to write sbrk in C and compile it to wasm. To do so, that C code can import an extern C function, emscripten_get_sbrk_ptr(), which basically just returns that location. The PostEmscripten pass can even apply that value at compile time, so we avoid the function call, and end up in the optimal place, see #2325 and emscripten PRs will be opened once other stuff lands. However, the SafeHeap pass must be updated to handle this, or our CI will break in the middle. This PR fixes that, basically making it check if env.DYNAMICTOP_PTR exists, or if not then looking for env.emscripten_get_sbrk_ptr, so that it can handle both.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The assumption is that an import
env.emscripten_get_sbrk_ptrexists, and we replace the value returned from there with a constant. (We can't do all this in wasm-emscripten-finalize, as it happens before the JS compiler runs, which can add more static allocations; we only know where the sbrk ptr is later in compilation.) This just replaces the import with a function returning the constant; inlining etc. can help more later.By setting this at compile time we can reduce code size and avoid importing it at runtime, which makes us more compatible with wasi (less special imports).