wasm2js: Avoid 64-bit scratch memory helpers in wasm-intrinsics #2926
Conversation
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
|
I'm wondering is it also could fix #2925 issue? |
|
@MaxGraey I doubt it's related. These intrinsics are used to do i64 division, remainder, etc., which I don't think is the case there. But you can test locally to verify if you want. |
tlively
approved these changes
Jun 23, 2020
src/passes/wasm-intrinsics.wat
Outdated
| @@ -11,6 +11,8 @@ | |||
| ;; special scratch space, wasm2js_scratch_load_i32 etc. | |||
| ;; * Fix function type of __wasm_ctz_i64, which was wrong somehow, | |||
| ;; i32, i32 => i32 instead of i64 => i64 | |||
| ;; * Add $wasm-intrinsics-temp-i64 global to avoid a 64-bit import | |||
| ;; wasm2js_scratch_load_i64/store | |||
There was a problem hiding this comment.
IIUC, this name won't exist in the codebase after this change, so it would probably be good to avoid using it in this comment.
There was a problem hiding this comment.
Sure, updated.
I kind of saw the lines here as a "history", but yeah, maybe that's confusing.
kripken
added a commit
to emscripten-core/emscripten
that referenced
this pull request
Jul 16, 2020
This was disabled due to some bugs with legalization, but those have been fixed meanwhile (in WebAssembly/binaryen#2926) so we can do this. This helps nicely, e.g. 6% shrinking of hello_webgl2_wasm2js in the minimal runtime size test. In addition to CI, verified with wasm2js3 locally.
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.
That code originally used memory location 0 to save 64 bits of
data (as that is what rust does apparently). We refactored it
manually to instead use a scratch memory helper, which is safer.
However, that 64-bit function ends up legalized, which actually
changes the interface between the module and the outside,
which is confusing and causes problems with optimizations
that can remove the getTempRet0 imports, see
emscripten-core/emscripten#11456
Instead, just use a global i64 to stash those bits. This requires
adding support for copying globals from the intrinsics module,
but otherwise seems simpler overall.
(tested on emscripten locally too)