Skip to content

Remove bulk memory instructions refering to active segments #2235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 19, 2019
Merged

Remove bulk memory instructions refering to active segments #2235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5247516
Remove bulk memory instructions refering to active segments
tlively Jul 19, 2019
71ddd85
Refinalize to prevent stale types
tlively Jul 19, 2019
a96a78c
Use parallel pass and preserve operands
tlively Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions src/passes/MemoryPacking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ struct MemoryPacking : public Pass {

if (module->features.hasBulkMemory()) {
// Remove any references to active segments that might be invalidated.
optimizeTrappingBulkMemoryOps(module);
optimizeTrappingBulkMemoryOps(runner, module);
// Conservatively refuse to change segments if any are passive to avoid
// invalidating segment indices or segment contents referenced from
// memory.init and data.drop instructions.
Expand Down Expand Up @@ -125,26 +125,36 @@ struct MemoryPacking : public Pass {
module->memory.segments.swap(packed);
}

void optimizeTrappingBulkMemoryOps(Module* module) {
struct Trapper : PostWalker<Trapper> {
bool changed = false;
void process(Expression* curr, Index index) {
if (!getModule()->memory.segments[index].isPassive) {
void optimizeTrappingBulkMemoryOps(PassRunner* runner, Module* module) {
struct Trapper : WalkerPass<PostWalker<Trapper>> {
bool changed;
void visitMemoryInit(MemoryInit* curr) {
if (!getModule()->memory.segments[curr->segment].isPassive) {
Builder builder(*getModule());
replaceCurrent(builder.blockify(builder.makeDrop(curr->dest),
builder.makeDrop(curr->offset),
builder.makeDrop(curr->size),
builder.makeUnreachable()));
changed = true;
}
}
void visitDataDrop(DataDrop* curr) {
if (!getModule()->memory.segments[curr->segment].isPassive) {
ExpressionManipulator::unreachable(curr);
changed = true;
}
}
void visitMemoryInit(MemoryInit* curr) { process(curr, curr->segment); }
void visitDataDrop(DataDrop* curr) { process(curr, curr->segment); }
void walkFunction(Function* func) {
changed = false;
PostWalker<Trapper>::walkFunction(func);
if (changed) {
ReFinalize().walkFunctionInModule(func, getModule());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it already a validation error to call memory.init or memory.drop on active segments?

Copy link
Member Author

@tlively tlively Jul 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened WebAssembly/bulk-memory-operations#107 to get some clarifation, but I guess its too late to change DataCount section now.

}
bool isFunctionParallel() override { return true; }
Pass* create() override { return new Trapper; }
} trapper;
trapper.walkModule(module);
trapper.run(runner, module);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops lol

}
};

Expand Down
13 changes: 12 additions & 1 deletion test/passes/memory-packing_all-features.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,18 @@
(type $FUNCSIG$v (func))
(memory $0 1 1)
(func $foo (; 0 ;) (type $FUNCSIG$v)
(unreachable)
(block
(drop
(i32.const 0)
)
(drop
(i32.const 0)
)
(drop
(i32.const 0)
)
(unreachable)
)
(unreachable)
)
(func $bar (; 1 ;) (type $FUNCSIG$v)
Expand Down