From b69f8a6bfc7963e0c56b10a69603e5dd36de58c3 Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Sun, 30 Jul 2017 16:55:06 -0700 Subject: don't remove values from breaks if the values have side effects --- src/passes/MergeBlocks.cpp | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) (limited to 'src/passes/MergeBlocks.cpp') diff --git a/src/passes/MergeBlocks.cpp b/src/passes/MergeBlocks.cpp index d32948bee..05aa468e1 100644 --- a/src/passes/MergeBlocks.cpp +++ b/src/passes/MergeBlocks.cpp @@ -73,14 +73,23 @@ namespace wasm { // For example, if there is a switch targeting us, we can't do it - we can't remove the value from other targets struct ProblemFinder : public ControlFlowWalker { Name origin; - bool foundSwitch = false; + bool noWay = false; // count br_ifs, and dropped br_ifs. if they don't match, then a br_if flow value is used, and we can't drop it Index brIfs = 0; Index droppedBrIfs = 0; + PassOptions& passOptions; + + ProblemFinder(PassOptions& passOptions) : passOptions(passOptions) {} void visitBreak(Break* curr) { - if (curr->name == origin && curr->condition) { - brIfs++; + if (curr->name == origin) { + if (curr->condition) { + brIfs++; + } + // if the value has side effects, we can't remove it + if (EffectAnalyzer(passOptions, curr->value).hasSideEffects()) { + noWay = true; + } } } @@ -94,12 +103,12 @@ struct ProblemFinder : public ControlFlowWalker { void visitSwitch(Switch* curr) { if (curr->default_ == origin) { - foundSwitch = true; + noWay = true; return; } for (auto& target : curr->targets) { if (target == origin) { - foundSwitch = true; + noWay = true; return; } } @@ -107,7 +116,7 @@ struct ProblemFinder : public ControlFlowWalker { bool found() { assert(brIfs >= droppedBrIfs); - return foundSwitch || brIfs > droppedBrIfs; + return noWay || brIfs > droppedBrIfs; } }; @@ -115,6 +124,9 @@ struct ProblemFinder : public ControlFlowWalker { // While doing so it can create new blocks, so optimize blocks as well. struct BreakValueDropper : public ControlFlowWalker { Name origin; + PassOptions& passOptions; + + BreakValueDropper(PassOptions& passOptions) : passOptions(passOptions) {} void visitBlock(Block* curr); @@ -143,7 +155,7 @@ struct BreakValueDropper : public ControlFlowWalker { }; // core block optimizer routine -static void optimizeBlock(Block* curr, Module* module) { +static void optimizeBlock(Block* curr, Module* module, PassOptions& passOptions) { bool more = true; bool changed = false; while (more) { @@ -159,14 +171,14 @@ static void optimizeBlock(Block* curr, Module* module) { if (child->name.is()) { Expression* expression = child; // check if it's ok to remove the value from all breaks to us - ProblemFinder finder; + ProblemFinder finder(passOptions); finder.origin = child->name; finder.walk(expression); if (finder.found()) { child = nullptr; } else { // fix up breaks - BreakValueDropper fixer; + BreakValueDropper fixer(passOptions); fixer.origin = child->name; fixer.setModule(module); fixer.walk(expression); @@ -217,7 +229,7 @@ static void optimizeBlock(Block* curr, Module* module) { } void BreakValueDropper::visitBlock(Block* curr) { - optimizeBlock(curr, getModule()); + optimizeBlock(curr, getModule(), passOptions); } struct MergeBlocks : public WalkerPass> { @@ -226,7 +238,7 @@ struct MergeBlocks : public WalkerPass> { Pass* create() override { return new MergeBlocks; } void visitBlock(Block *curr) { - optimizeBlock(curr, getModule()); + optimizeBlock(curr, getModule(), getPassOptions()); } Block* optimize(Expression* curr, Expression*& child, Block* outer = nullptr, Expression** dependency1 = nullptr, Expression** dependency2 = nullptr) { -- cgit v1.2.3 From bfdbc0c3f6a835231b218a60ddd6b52f7e9affed Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Mon, 31 Jul 2017 13:39:46 -0700 Subject: review comments --- src/passes/MergeBlocks.cpp | 10 +++++----- src/passes/OptimizeInstructions.cpp | 10 +++++----- src/wasm/wasm-binary.cpp | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) (limited to 'src/passes/MergeBlocks.cpp') diff --git a/src/passes/MergeBlocks.cpp b/src/passes/MergeBlocks.cpp index 05aa468e1..7c086a920 100644 --- a/src/passes/MergeBlocks.cpp +++ b/src/passes/MergeBlocks.cpp @@ -73,7 +73,7 @@ namespace wasm { // For example, if there is a switch targeting us, we can't do it - we can't remove the value from other targets struct ProblemFinder : public ControlFlowWalker { Name origin; - bool noWay = false; + bool foundProblem = false; // count br_ifs, and dropped br_ifs. if they don't match, then a br_if flow value is used, and we can't drop it Index brIfs = 0; Index droppedBrIfs = 0; @@ -88,7 +88,7 @@ struct ProblemFinder : public ControlFlowWalker { } // if the value has side effects, we can't remove it if (EffectAnalyzer(passOptions, curr->value).hasSideEffects()) { - noWay = true; + foundProblem = true; } } } @@ -103,12 +103,12 @@ struct ProblemFinder : public ControlFlowWalker { void visitSwitch(Switch* curr) { if (curr->default_ == origin) { - noWay = true; + foundProblem = true; return; } for (auto& target : curr->targets) { if (target == origin) { - noWay = true; + foundProblem = true; return; } } @@ -116,7 +116,7 @@ struct ProblemFinder : public ControlFlowWalker { bool found() { assert(brIfs >= droppedBrIfs); - return noWay || brIfs > droppedBrIfs; + return foundProblem || brIfs > droppedBrIfs; } }; diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp index 6179833b8..4fd7cbe8d 100644 --- a/src/passes/OptimizeInstructions.cpp +++ b/src/passes/OptimizeInstructions.cpp @@ -884,15 +884,15 @@ private: if (!Properties::emitsBoolean(left) || !Properties::emitsBoolean(right)) return nullptr; auto leftEffects = EffectAnalyzer(getPassOptions(), left); auto rightEffects = EffectAnalyzer(getPassOptions(), right); - auto leftSideEffects = leftEffects.hasSideEffects(); - auto rightSideEffects = rightEffects.hasSideEffects(); - if (leftSideEffects && rightSideEffects) return nullptr; // both must execute + auto leftHasSideEffects = leftEffects.hasSideEffects(); + auto rightHasSideEffects = rightEffects.hasSideEffects(); + if (leftHasSideEffects && rightHasSideEffects) return nullptr; // both must execute // canonicalize with side effects, if any, happening on the left - if (rightSideEffects) { + if (rightHasSideEffects) { if (CostAnalyzer(left).cost < MIN_COST) return nullptr; // avoidable code is too cheap if (leftEffects.invalidates(rightEffects)) return nullptr; // cannot reorder std::swap(left, right); - } else if (leftSideEffects) { + } else if (leftHasSideEffects) { if (CostAnalyzer(right).cost < MIN_COST) return nullptr; // avoidable code is too cheap } else { // no side effects, reorder based on cost estimation diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp index ac8df5711..1bc5ada94 100644 --- a/src/wasm/wasm-binary.cpp +++ b/src/wasm/wasm-binary.cpp @@ -2073,7 +2073,7 @@ void WasmBinaryBuilder::visitBlock(Block *curr) { auto* item = expressionStack[i]; curr->list.push_back(item); if (i < end - 1) { - // stacky&unreachable code may introduce elements that need to be dropped in non-final positoins + // stacky&unreachable code may introduce elements that need to be dropped in non-final positions if (isConcreteWasmType(item->type)) { curr->list.back() = Builder(wasm).makeDrop(curr->list.back()); } -- cgit v1.2.3