Added codegen for memory.size and memory.grow instructions #14
Conversation
|
I would prefix both these functions with "instruction_" for now. I see this builds off your other PR Sorry for not being fast with the reviews on these, working very slowly on switching away from llvm_alt |
|
Will update. No worries on the speed. I'm used to PRs dripping through review processes like a coffee filter. Once I can get changes in PR form and presented to you guys I'm happy to have it sit until needed. It's unfortunate GitHub doesn't render small inter-dependent PRs nicer. |
|
Reading through these changes--all looks good. Will rereview after the PRs this one is based off are merged :) |
|
@geky can you rebase this off master? |
These simply call the externally linked instruction_memory_grow and instruction_memory_size functions in the runtime. Once connected, these runtime functions can use the existing expand_memory logic that is specific to each runtime-memory implementation.
Note this is only needed when incrementally compiling, which we don't do because of LTO.
geky
force-pushed
the
memory-instructions
branch
from
fe454f8 to
59e68b4
Compare
| ImportSectionEntryType::Memory(memory_ty) => { | ||
| self.memories.push(*memory_ty); | ||
| } | ||
| ImportSectionEntryType::Table(table_ty) => { | ||
| self.tables.push(*table_ty); | ||
| } |
There was a problem hiding this comment.
Kinda nervous about this, as we don't actually support additional memories or tables. I suppose I can't think of anywhere else in the code we assume this (or at least nowhere that'd cause subtle unsafety)
There was a problem hiding this comment.
I believe these were needed because clang was emitting empty imports. They were empty, but aWsm was failing to parse the empty stubs.
Though I will admit there was a bit of poking-the-code-until-it-works here.
There was a problem hiding this comment.
It does match the process_memory_section/process_table_section function's behavior:
Line 938 in 59e68b4
| Operator::MemorySize { .. } => Instruction::MemorySize, | ||
| Operator::MemoryGrow { .. } => Instruction::MemoryGrow, |
There was a problem hiding this comment.
Are there no useful parameters to pipe down?
There was a problem hiding this comment.
Nope, the Wasm spec has a byte reserved for "memory index" which is currently unused
https://webassembly.github.io/spec/core/binary/instructions.html#memory-instructions
Note
In future versions of WebAssembly, the additional zero bytes occurring in the encoding of the memory.size
and memory.grow instructions may be used to index additional memories.
You could pipe down this byte, but then the unused variable would move into more places in the code base.
| i32 instruction_memory_size() { | ||
| return memory_size / WASM_PAGE_SIZE; | ||
| } | ||
|
|
||
| i32 instruction_memory_grow(i32 count) { | ||
| i32 prev_size = instruction_memory_size(); | ||
| for (int i = 0; i < count; i++) { | ||
| expand_memory(); | ||
| } | ||
|
|
||
| return prev_size; | ||
| } | ||
|
|
There was a problem hiding this comment.
In retrospect, I really should've abstracted this repeated code out... this new addition is in line with everything else tho, so keep it this way
| i32 instruction_memory_size() { | ||
| return memory_size / WASM_PAGE_SIZE; | ||
| } |
There was a problem hiding this comment.
We need to be careful here. If you look at the mmap implementation, technically there is some support for MMAP granularity smaller than wasm page size granularity. This could lead to surprising behavior from using this (seemingly simple) function, and we should at least document that.
Long term we probably should decide if we want to support the mmap hack.
Creating PR for review, currently building off of #12, happy to rebase on what changes goes in. Only the last commit should be relevant: fc2bbe5
I tried to follow the implementation of instruction->runtime function instructions, so hopefully this is better than my previous attempt (#2).
Currently linking to
memory_growandmemory_size_. Need to fix the name conflict with thememory_sizevariable, though happy to take alternative names.Have we considered prefixing all of the runtime functions with something like
awsmr_memory_grow?