Remove bulk memory instructions refering to active segments #2235
Conversation
This prevents those instructions from becoming invalid due to memory packing optimizations and is also a code size win. Fixes WebAssembly#2227.
| void process(Expression* curr, Index index) { | ||
| if (!getModule()->memory.segments[index].isPassive) { | ||
| ExpressionManipulator::unreachable(curr); | ||
| } |
There was a problem hiding this comment.
Isn't it already a validation error to call memory.init or memory.drop on active segments?
There was a problem hiding this comment.
Nope, for some reason that is valid but traps (because the segments were dropped during initialization). According the bulk memory proposal's overview.md, anyway.
There was a problem hiding this comment.
Oh I see. I guess that is because validation is done with only the information in the data count section which is not enough to know about the active/passive status of as segment. Otherwise it would make sense to be a validation error.
Why would we ever expect to have such instruction in the input? Perhaps binaryen could make this a hard error rather than delaying the error until runtime?
There was a problem hiding this comment.
Yeah I briefly discussed the possibility of making this a binaryen validation error with @kripken, but I don't believe we have precedent for Binaryen rejecting valid wasm modules, even if they do silly things. We decided this would be better because it still accepts all valid modules and is a nice size optimization, too.
There was a problem hiding this comment.
I opened WebAssembly/bulk-memory-operations#107 to get some clarifation, but I guess its too late to change DataCount section now.
src/passes/MemoryPacking.cpp
Outdated
|
|
||
| void optimizeTrappingBulkMemoryOps(Module* module) { | ||
| struct Trapper : PostWalker<Trapper> { | ||
| bool changed = false; |
src/passes/MemoryPacking.cpp
Outdated
| @@ -120,6 +124,28 @@ struct MemoryPacking : public Pass { | |||
| } | |||
| module->memory.segments.swap(packed); | |||
| } | |||
|
|
|||
| void optimizeTrappingBulkMemoryOps(Module* module) { | |||
| struct Trapper : PostWalker<Trapper> { | |||
There was a problem hiding this comment.
it's not urgent but this should be parallelized. please add at least a TODO about that
| } trapper; | ||
| trapper.walkModule(module); | ||
| trapper.run(runner, module); |
There was a problem hiding this comment.
this is confusing actually - this just does the normal walk. I should add an assertion about it. To parallelize you need to create a PassRunner, add the pass, and run that. Should also set isNested. See for example Inlining which has nested passes.
|
@kripken is this good to go with your separate changes to make |
|
Yes, lgtm! |
|
@tlively this broke tests when it landed, stuff like Oddly it didn't fail in your PR, not sure why. |
|
Gross, it was probably a bad merge interaction with something else that landed. I’ll revert and debug. |
This prevents those instructions from becoming invalid due to memory
packing optimizations and is also a code size win. Fixes #2227.