From 7a06499ada2f5d0d349731595b3a09a46919c443 Mon Sep 17 00:00:00 2001 From: "Alon Zakai (kripken)" Date: Fri, 28 Jul 2017 16:26:46 -0700 Subject: fix binary emitting of untaken branches, and also handle reading of unreachable stacky code which may introduce concrete elements in non-final block positoins --- src/wasm/wasm-binary.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'src/wasm/wasm-binary.cpp') diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp index 2df7f2e26..ac8df5711 100644 --- a/src/wasm/wasm-binary.cpp +++ b/src/wasm/wasm-binary.cpp @@ -545,7 +545,7 @@ void WasmBinaryWriter::recurse(Expression*& curr) { } static bool brokenTo(Block* block) { - return block->name.is() && BranchUtils::BranchSeeker::has(block, block->name); + return block->name.is() && BranchUtils::BranchSeeker::hasNamed(block, block->name); } void WasmBinaryWriter::visitBlock(Block *curr) { @@ -636,7 +636,7 @@ int32_t WasmBinaryWriter::getBreakIndex(Name name) { // -1 if not found return breakStack.size() - 1 - i; } } - std::cerr << "bad break: " << name << std::endl; + std::cerr << "bad break: " << name << " in " << currFunction->name << std::endl; abort(); } @@ -2070,7 +2070,14 @@ void WasmBinaryBuilder::visitBlock(Block *curr) { } for (size_t i = start; i < end; i++) { if (debug) std::cerr << " " << size_t(expressionStack[i]) << "\n zz Block element " << curr->list.size() << std::endl; - curr->list.push_back(expressionStack[i]); + 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 + if (isConcreteWasmType(item->type)) { + curr->list.back() = Builder(wasm).makeDrop(curr->list.back()); + } + } } expressionStack.resize(start); curr->finalize(curr->type); -- 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/wasm/wasm-binary.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