From a2e170294c03d5fe0d91e2797d96acdb95b1d37c Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 17 Jul 2017 10:23:06 -0700 Subject: fix blockifyMerge logic - it needs to not skip code in the block we merge to. since that's a fairly specific functionality needed in removeUnusedBrs, move it to there --- src/passes/RemoveUnusedBrs.cpp | 32 ++++++++++++++++++++++++++++++-- src/wasm-builder.h | 22 ---------------------- 2 files changed, 30 insertions(+), 24 deletions(-) (limited to 'src') diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp index b725de901..3ab0371ed 100644 --- a/src/passes/RemoveUnusedBrs.cpp +++ b/src/passes/RemoveUnusedBrs.cpp @@ -232,11 +232,39 @@ struct RemoveUnusedBrs : public WalkerPass> { // this is already an if-else. if one side is a dead end, we can append to the other, if // there is no returned value to concern us assert(!isConcreteWasmType(iff->type)); // can't be, since in the middle of a block + + // ensures the first node is a block, if it isn't already, and merges in the second, + // either as a single element or, if a block, by appending to the first block. this + // keeps the order of operations in place, that is, the appended element will be + // executed after the first node's elements + auto blockifyMerge = [&](Expression* any, Expression* append) -> Block* { + Block* block = nullptr; + if (any) block = any->dynCast(); + // if the first isn't a block, or it's a block with a name (so we might + // branch to the end, and so can't append to it, we might skip that code!) + // then make a new block + if (!block || block->name.is()) { + block = builder.makeBlock(any); + } else { + assert(!isConcreteWasmType(block->type)); + } + auto* other = append->dynCast(); + if (!other) { + block->list.push_back(append); + } else { + for (auto* item : other->list) { + block->list.push_back(item); + } + } + block->finalize(); + return block; + }; + if (ExpressionAnalyzer::obviouslyDoesNotFlowOut(iff->ifTrue)) { - iff->ifFalse = builder.blockifyMerge(iff->ifFalse, builder.stealSlice(block, i + 1, list.size())); + iff->ifFalse = blockifyMerge(iff->ifFalse, builder.stealSlice(block, i + 1, list.size())); return true; } else if (ExpressionAnalyzer::obviouslyDoesNotFlowOut(iff->ifFalse)) { - iff->ifTrue = builder.blockifyMerge(iff->ifTrue, builder.stealSlice(block, i + 1, list.size())); + iff->ifTrue = blockifyMerge(iff->ifTrue, builder.stealSlice(block, i + 1, list.size())); return true; } } diff --git a/src/wasm-builder.h b/src/wasm-builder.h index 41aaf6766..f702342d2 100644 --- a/src/wasm-builder.h +++ b/src/wasm-builder.h @@ -315,28 +315,6 @@ public: return block; } - // ensures the first node is a block, if it isn't already, and merges in the second, - // either as a single element or, if a block, by appending to the first block - Block* blockifyMerge(Expression* any, Expression* append) { - Block* block = nullptr; - if (any) block = any->dynCast(); - if (!block) { - block = makeBlock(any); - } else { - assert(!isConcreteWasmType(block->type)); - } - auto* other = append->dynCast(); - if (!other) { - block->list.push_back(append); - } else { - for (auto* item : other->list) { - block->list.push_back(item); - } - } - block->finalize(); - return block; - } - // a helper for the common pattern of a sequence of two expressions. Similar to // blockify, but does *not* reuse a block if the first is one. Block* makeSequence(Expression* left, Expression* right) { -- cgit v1.2.3 From d1b598c542d174bd9a55d0edcdfaf27948a140dd Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 17 Jul 2017 10:23:42 -0700 Subject: add missing finalizations in removeUnusedBrs, when we change an if side and its outer block, we need to finalize the if first and then the block containing it --- src/passes/RemoveUnusedBrs.cpp | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'src') diff --git a/src/passes/RemoveUnusedBrs.cpp b/src/passes/RemoveUnusedBrs.cpp index 3ab0371ed..9fed3e4a5 100644 --- a/src/passes/RemoveUnusedBrs.cpp +++ b/src/passes/RemoveUnusedBrs.cpp @@ -226,6 +226,8 @@ struct RemoveUnusedBrs : public WalkerPass> { // we need the ifTrue to break, so it cannot reach the code we want to move if (ExpressionAnalyzer::obviouslyDoesNotFlowOut(iff->ifTrue)) { iff->ifFalse = builder.stealSlice(block, i + 1, list.size()); + iff->finalize(); + block->finalize(); return true; } } else { @@ -262,9 +264,13 @@ struct RemoveUnusedBrs : public WalkerPass> { if (ExpressionAnalyzer::obviouslyDoesNotFlowOut(iff->ifTrue)) { iff->ifFalse = blockifyMerge(iff->ifFalse, builder.stealSlice(block, i + 1, list.size())); + iff->finalize(); + block->finalize(); return true; } else if (ExpressionAnalyzer::obviouslyDoesNotFlowOut(iff->ifFalse)) { iff->ifTrue = blockifyMerge(iff->ifTrue, builder.stealSlice(block, i + 1, list.size())); + iff->finalize(); + block->finalize(); return true; } } -- cgit v1.2.3 From 557eca7055ba7b0546624006ddac9eed544a2647 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 17 Jul 2017 10:44:34 -0700 Subject: fix vacuum bug on nop'ing an if whose body has brs that cause type changes when removed --- src/passes/Vacuum.cpp | 1 + test/passes/vacuum.txt | 4 ++++ test/passes/vacuum.wast | 23 +++++++++++++++++++++++ 3 files changed, 28 insertions(+) (limited to 'src') diff --git a/src/passes/Vacuum.cpp b/src/passes/Vacuum.cpp index 3f27fb36a..bb6c7c296 100644 --- a/src/passes/Vacuum.cpp +++ b/src/passes/Vacuum.cpp @@ -225,6 +225,7 @@ struct Vacuum : public WalkerPass> { child = curr->ifFalse; typeUpdater.noteRecursiveRemoval(curr->ifTrue); } else { + typeUpdater.noteRecursiveRemoval(curr); ExpressionManipulator::nop(curr); return; } diff --git a/test/passes/vacuum.txt b/test/passes/vacuum.txt index f3b06af1a..011fb99f5 100644 --- a/test/passes/vacuum.txt +++ b/test/passes/vacuum.txt @@ -301,4 +301,8 @@ ) ) ) + (func $nop-if-type-changes (type $0) + (local $0 i32) + (nop) + ) ) diff --git a/test/passes/vacuum.wast b/test/passes/vacuum.wast index db4bf6749..3b5b22e9c 100644 --- a/test/passes/vacuum.wast +++ b/test/passes/vacuum.wast @@ -637,4 +637,27 @@ (i32.const 1579493952) ) ) + (func $nop-if-type-changes (type $0) + (local $0 i32) + (block $label$0 + (if + (i32.eqz + (get_local $0) + ) + (block $label$1 + (block + (if ;; we nop this if, which has a type change for block $label$1, no more brs to it + (i32.const 0) + (br_if $label$1 + (i32.const 1717966400) + ) + ) + (drop + (br $label$0) + ) + ) + ) + ) + ) + ) ) -- cgit v1.2.3 From 2daf000f471afdcf21f784f83440bb4bdc1a9de9 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Mon, 17 Jul 2017 11:16:49 -0700 Subject: fix off-by-one in assertion in optimize-instructions --- src/passes/OptimizeInstructions.cpp | 2 +- test/passes/optimize-instructions.txt | 13 +++++++++++++ test/passes/optimize-instructions.wast | 16 ++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp index c6006d712..de6f96ccc 100644 --- a/src/passes/OptimizeInstructions.cpp +++ b/src/passes/OptimizeInstructions.cpp @@ -453,7 +453,7 @@ struct OptimizeInstructions : public WalkerPass 0 && count < 32 - bits) || (constSignBit && count == 0)) { // mixed or [zero upper const bits with sign bit set]; the compared values can never be identical, so // force something definitely impossible even after zext - assert(bits < 31); + assert(bits < 32); c->value = Literal(int32_t(0x80000000)); // TODO: if no side effects, we can just replace it all with 1 or 0 } else { diff --git a/test/passes/optimize-instructions.txt b/test/passes/optimize-instructions.txt index 8b42fd2ad..604c9c3ea 100644 --- a/test/passes/optimize-instructions.txt +++ b/test/passes/optimize-instructions.txt @@ -1993,4 +1993,17 @@ ) ) ) + (func $sign-ext-1-and-ne (type $2) (result i32) + (select + (i32.ne + (i32.and + (call $sign-ext-1-and-ne) + (i32.const 2147483647) + ) + (i32.const -2147483648) + ) + (i32.const 2) + (i32.const 1) + ) + ) ) diff --git a/test/passes/optimize-instructions.wast b/test/passes/optimize-instructions.wast index 6d1fd60ed..cf8275412 100644 --- a/test/passes/optimize-instructions.wast +++ b/test/passes/optimize-instructions.wast @@ -2417,4 +2417,20 @@ ) ) ) + (func $sign-ext-1-and-ne (result i32) + (select + (i32.ne + (i32.const 1333788672) + (i32.shr_s + (i32.shl + (call $sign-ext-1-and-ne) + (i32.const 1) + ) + (i32.const 1) + ) + ) + (i32.const 2) + (i32.const 1) + ) + ) ) -- cgit v1.2.3