wasm2js: Support for exported memory #3323
Conversation
sbc100
force-pushed
the
exported_memory
branch
3 times, most recently
from
6277f12 to
5333969
Compare
sbc100
force-pushed
the
exported_memory
branch
from
5333969 to
10eb441
Compare
|
I think this good to land now. |
src/wasm2js.h
Outdated
| ast->push_back( | ||
| ValueBuilder::makeBinary(ValueBuilder::makeName("bufferView"), | ||
| SET, | ||
| ValueBuilder::makeName(HEAPU8))); |
There was a problem hiding this comment.
this creation of bufferView looks unconditional - whenever there is a memory at all. why is another one needed elsewhere?
There was a problem hiding this comment.
This is not a creation of anything but an assignment to the outer scoped bufferView
But you are right I think it needs to be conditional in the same way that bufferView itself is conditionally declared in the outer scope.
There was a problem hiding this comment.
Oh, I see, I was misreading it, sorry.
|
|
||
| var memasmFunc = new ArrayBuffer(1507328); | ||
| var bufferView = new Uint8Array(memasmFunc); | ||
| var bufferView; |
There was a problem hiding this comment.
can we not remove this bufferView entirely, and just use HEAP8 which is guaranteed to exist?
There was a problem hiding this comment.
See my TODO: We should probably just share a single HEAPU8 var. above.
I was thinking that could be a followup. Keeping this change slightly more limited.
There was a problem hiding this comment.
This does make it harder to review, though, see the comment above with unconditional - it looks like the current PR is not emitting bufferView properly, but I may be missing something?
There was a problem hiding this comment.
If we start to share HEAPU8 with the containing scope is that going to interfere with the emscripten-side that also defines a HEAPU8 itself? I mean that should always point to the same thing anyway so maybe its fine?
There was a problem hiding this comment.
Yes it looks like the wasm2js output shares a scope with the rest of emscripten, but asmFunc currently creates it own function-local versions of HEAP8, etc. This means that in emscripten output there are two copies of all these views today.
We should possibly merge those two (i.e. have the output HEAP8 be the same as the inner HEAP8), but I'd rather not worry about that as part of this change.
The asmFunc now sets the extern `bufferView` variable as well as its own internal views.
sbc100
force-pushed
the
exported_memory
branch
from
10eb441 to
a34937d
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
This is fairly confusing (not because of you, but because the wasm2js output is complex with the different modes) but reading the outputs I've convinced myself it is correct.
This captures some minor code size win from (I belive) WebAssembly/binaryen#3323
Now that wasm2js supports memories defined on the wasm side this seems to just work. See: WebAssembly/binaryen#3323
This captures some minor code size win from (I belive) WebAssembly/binaryen#3323
This captures some minor code size win from (I belive) WebAssembly/binaryen#3323
Now that wasm2js supports memories defined on the wasm side this seems to just work. See: WebAssembly/binaryen#3323
Now that wasm2js supports memories defined on the wasm side this seems to just work. See: WebAssembly/binaryen#3323
This change is required for emscripten-core/emscripten#12323