From e55c5e96db2601f6305c91ee9e7bf6d1ab8d9ef7 Mon Sep 17 00:00:00 2001 From: Ashley Nelson Date: Thu, 7 Dec 2023 06:21:40 +0000 Subject: [PATCH 1/3] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?= =?UTF-8?q?anges=20to=20main=20this=20commit=20is=20based=20on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.4 [skip ci] --- src/passes/Outlining.cpp | 57 +++++--- src/passes/stringify-walker-impl.h | 17 ++- src/passes/stringify-walker.h | 14 +- src/wasm/wasm-ir-builder.cpp | 4 +- test/lit/passes/outlining.wast | 223 ++++++++++++++++++++++++++++- 5 files changed, 282 insertions(+), 33 deletions(-) diff --git a/src/passes/Outlining.cpp b/src/passes/Outlining.cpp index f799d2c7adb..345b3f9a2da 100644 --- a/src/passes/Outlining.cpp +++ b/src/passes/Outlining.cpp @@ -45,8 +45,8 @@ struct ReconstructStringifyWalker ReconstructStringifyWalker(Module* wasm) : existingBuilder(*wasm), outlinedBuilder(*wasm) { this->setModule(wasm); - DBG(std::cout << "\n\nexistingBuilder: " << &existingBuilder - << " outlinedBuilder: " << &outlinedBuilder); + DBG(std::cerr << "\nexistingBuilder: " << &existingBuilder + << " outlinedBuilder: " << &outlinedBuilder << "\n"); } // As we reconstruct the IR during outlining, we need to know what @@ -91,7 +91,7 @@ struct ReconstructStringifyWalker DBG(std::string desc); if (auto curr = reason.getBlockStart()) { ASSERT_OK(existingBuilder.visitBlockStart(curr->block)); - DBG(desc = "Block Start for "); + DBG(desc = "Block Start at "); } else if (auto curr = reason.getIfStart()) { // IR builder needs the condition of the If pushed onto the builder before // visitIfStart(), which will expect to be able to pop the condition. @@ -99,10 +99,25 @@ struct ReconstructStringifyWalker // onto the If when the outer scope was visited. existingBuilder.push(curr->iff->condition); ASSERT_OK(existingBuilder.visitIfStart(curr->iff)); - DBG(desc = "If Start for "); + DBG(desc = "If Start at "); + } else if (reason.getElseStart()) { + ASSERT_OK(existingBuilder.visitElse()); + DBG(desc = "Else Start at "); + } else if (auto curr = reason.getLoopStart()) { + ASSERT_OK(existingBuilder.visitLoopStart(curr->loop)); + DBG(desc = "Loop Start at "); } else if (reason.getEnd()) { ASSERT_OK(existingBuilder.visitEnd()); - DBG(desc = "End for "); + // Outlining performs an unnested walk of the Wasm module, visiting + // each scope one at a time. IRBuilder, in contrast, expects to + // visit several nested scopes at a time. Thus, calling end() finalizes + // the control flow and places it on IRBuilder's internal stack, ready for + // the enclosing scope to consume its expressions off the stack. Since + // outlining walks unnested, the enclosing scope never arrives to retrieve + // its expressions off the stack, so we must call build() after visitEnd() + // to clear the internal stack IRBuilder manages. + ASSERT_OK(existingBuilder.build()); + DBG(desc = "End at "); } else { DBG(desc = "addUniqueSymbol for unimplemented control flow "); WASM_UNREACHABLE("unimplemented control flow"); @@ -133,8 +148,9 @@ struct ReconstructStringifyWalker instrCounter = 0; seqCounter = 0; state = NotInSeq; - DBG(std::cout << "\n\n$" << func->name << " Func Start " - << &existingBuilder); + DBG(std::cerr << "\n" + << "Func Start to $" << func->name << " at " + << &existingBuilder << "\n"); } ReconstructState getCurrState() { @@ -185,18 +201,18 @@ struct ReconstructStringifyWalker // Make a call from the existing function to the outlined function. This // call will replace the instructions moved to the outlined function. ASSERT_OK(existingBuilder.makeCall(outlinedFunc->name, false)); - DBG(std::cout << "\ncreated outlined fn: " << outlinedFunc->name); + DBG(std::cerr << "\ncreated outlined fn: " << outlinedFunc->name << "\n"); } void transitionToInSkipSeq() { Function* outlinedFunc = getModule()->getFunction(sequences[seqCounter].func); ASSERT_OK(existingBuilder.makeCall(outlinedFunc->name, false)); - DBG(std::cout << "\n\nstarting to skip instructions " + DBG(std::cerr << "\nstarting to skip instructions " << sequences[seqCounter].startIdx << " - " << sequences[seqCounter].endIdx - 1 << " to " << sequences[seqCounter].func - << " and adding call() instead"); + << " and adding call() instead\n"); } void maybeEndSeq() { @@ -207,19 +223,20 @@ struct ReconstructStringifyWalker } void transitionToNotInSeq() { + DBG(std::cerr << "End of sequence "); if (state == InSeq) { ASSERT_OK(outlinedBuilder.visitEnd()); - DBG(std::cout << "\n\nEnd of sequence to " << &outlinedBuilder); + DBG(std::cerr << "to " << &outlinedBuilder); } + DBG(std::cerr << "\n\n"); // Completed a sequence so increase the seqCounter and reset the state. seqCounter++; } #if OUTLINING_DEBUG void printAddUniqueSymbol(std::string desc) { - std::cout << "\n" - << desc << std::to_string(instrCounter) << " to " - << &existingBuilder; + std::cerr << desc << std::to_string(instrCounter) << " to " + << &existingBuilder << "\n"; } void printVisitExpression(Expression* curr) { @@ -227,9 +244,9 @@ struct ReconstructStringifyWalker : state == NotInSeq ? &existingBuilder : nullptr; auto verb = state == InSkipSeq ? "skipping " : "adding "; - std::cout << "\n" - << verb << std::to_string(instrCounter) << ": " - << ShallowExpression{curr} << " to " << builder; + std::cerr << verb << std::to_string(instrCounter) << ": " + << ShallowExpression{curr} << "(" << curr << ") to " << builder + << "\n"; } #endif }; @@ -349,14 +366,14 @@ struct Outlining : public Pass { #if OUTLINING_DEBUG void printHashString(const std::vector& hashString, const std::vector& exprs) { - std::cout << "\n\n"; + std::cerr << "\n\n"; for (Index idx = 0; idx < hashString.size(); idx++) { Expression* expr = exprs[idx]; if (expr) { - std::cout << idx << " - " << hashString[idx] << ": " + std::cerr << idx << " - " << hashString[idx] << ": " << ShallowExpression{expr} << "\n"; } else { - std::cout << idx << ": unique symbol\n"; + std::cerr << idx << ": unique symbol\n"; } } } diff --git a/src/passes/stringify-walker-impl.h b/src/passes/stringify-walker-impl.h index 8ed166d7541..e1c01c115dd 100644 --- a/src/passes/stringify-walker-impl.h +++ b/src/passes/stringify-walker-impl.h @@ -16,6 +16,14 @@ #include "stringify-walker.h" +#define STRINGIFY_DEBUG 0 + +#if STRINGIFY_DEBUG +#define DBG(statement) statement +#else +#define DBG(statement) +#endif + #ifndef wasm_passes_stringify_walker_impl_h #define wasm_passes_stringify_walker_impl_h @@ -43,7 +51,7 @@ template inline void StringifyWalker::scan(SubType* self, Expression** currp) { Expression* curr = *currp; if (Properties::isControlFlowStructure(curr)) { - self->controlFlowQueue.push(currp); + self->controlFlowQueue.push(curr); self->pushTask(doVisitExpression, currp); // The if-condition is a value child consumed by the if control flow, which // makes the if-condition a true sibling rather than part of its contents in @@ -60,9 +68,10 @@ inline void StringifyWalker::scan(SubType* self, Expression** currp) { // of control flow. template void StringifyWalker::dequeueControlFlow() { auto& queue = controlFlowQueue; - Expression** currp = queue.front(); + Expression* curr = queue.front(); queue.pop(); - Expression* curr = *currp; + DBG(std::cerr << "controlFlowQueue.pop: " << ShallowExpression{curr} << ", " + << curr << "\n"); // TODO: Issue #5796, Make a ControlChildIterator switch (curr->_id) { @@ -80,7 +89,7 @@ template void StringifyWalker::dequeueControlFlow() { addUniqueSymbol(SeparatorReason::makeIfStart(iff)); Super::walk(iff->ifTrue); if (iff->ifFalse) { - addUniqueSymbol(SeparatorReason::makeElseStart(iff)); + addUniqueSymbol(SeparatorReason::makeElseStart()); Super::walk(iff->ifFalse); } addUniqueSymbol(SeparatorReason::makeEnd()); diff --git a/src/passes/stringify-walker.h b/src/passes/stringify-walker.h index eb4a4482b2c..6f50f58d40a 100644 --- a/src/passes/stringify-walker.h +++ b/src/passes/stringify-walker.h @@ -82,9 +82,7 @@ struct StringifyWalker If* iff; }; - struct ElseStart { - If* iff; - }; + struct ElseStart {}; struct LoopStart { Loop* loop; @@ -119,8 +117,8 @@ struct StringifyWalker static SeparatorReason makeIfStart(If* iff) { return SeparatorReason(IfStart{iff}); } - static SeparatorReason makeElseStart(If* iff) { - return SeparatorReason(ElseStart{iff}); + static SeparatorReason makeElseStart() { + return SeparatorReason(ElseStart{}); } static SeparatorReason makeLoopStart(Loop* loop) { return SeparatorReason(LoopStart{loop}); @@ -170,7 +168,11 @@ struct StringifyWalker return o << "~~~Undefined in operator<< overload~~~"; } - std::queue controlFlowQueue; + // To ensure control flow children are walked consistently during outlining, + // we push a copy of the control flow expression. This avoids an issue where + // control flow no longer points to the same expression after being + // outlined into a new function. + std::queue controlFlowQueue; /* * To initiate the walk, subclasses should call walkModule with a pointer to diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 5bc2c15baa4..be2bb440416 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -154,7 +154,7 @@ void IRBuilder::push(Expression* expr) { } scope.exprStack.push_back(expr); - DBG(std::cerr << "After pushing " << ShallowExpression(expr) << ":\n"); + DBG(std::cerr << "After pushing " << ShallowExpression{expr} << ":\n"); DBG(dump()); } @@ -253,7 +253,7 @@ void IRBuilder::dump() { std::cerr << ":\n"; for (auto* expr : scope.exprStack) { - std::cerr << " " << ShallowExpression(expr) << "\n"; + std::cerr << " " << ShallowExpression{expr} << "\n"; } } #endif // IR_BUILDER_DEBUG diff --git a/test/lit/passes/outlining.wast b/test/lit/passes/outlining.wast index ad712b31a4c..1c5ec06bdc3 100644 --- a/test/lit/passes/outlining.wast +++ b/test/lit/passes/outlining.wast @@ -238,7 +238,59 @@ ) ) -;; Tests that outlining works correctly with If control flow +;; Tests that outlining works correctly with if-condition +(module + ;; CHECK: (type $0 (func)) + + ;; CHECK: (type $1 (func (result i32))) + + ;; CHECK: (global $global$1 (mut i32) (i32.const 100)) + (global $global$1 (mut i32) (i32.const 100)) + ;; CHECK: (func $outline$ (type $1) (result i32) + ;; CHECK-NEXT: (i32.eqz + ;; CHECK-NEXT: (global.get $global$1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + + ;; CHECK: (func $a (type $0) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: (global.set $global$1 + ;; CHECK-NEXT: (i32.const 15) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $a + (if + (i32.eqz + (global.get $global$1) + ) + (global.set $global$1 + (i32.const 15) + ) + ) + ) + ;; CHECK: (func $b (type $0) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: (global.set $global$1 + ;; CHECK-NEXT: (i32.const 20) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $b + (if + (i32.eqz + (global.get $global$1) + ) + (global.set $global$1 + (i32.const 20) + ) + ) + ) +) + +;; Outline if-true. (module ;; CHECK: (type $0 (func (param i32))) @@ -288,6 +340,138 @@ ) ) +;; Outline if-false. +(module + ;; CHECK: (type $0 (func)) + + ;; CHECK: (global $global$1 (mut i32) (i32.const 100)) + (global $global$1 (mut i32) (i32.const 100)) + ;; CHECK: (func $outline$ (type $0) + ;; CHECK-NEXT: (global.set $global$1 + ;; CHECK-NEXT: (i32.const 100) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + + ;; CHECK: (func $a (type $0) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.eqz + ;; CHECK-NEXT: (global.get $global$1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.set $global$1 + ;; CHECK-NEXT: (i32.const 15) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $a + (if + (i32.eqz + (global.get $global$1) + ) + (global.set $global$1 + (i32.const 15) + ) + (block + (global.set $global$1 + (i32.const 100) + ) + ) + ) + ) + ;; CHECK: (func $b (type $0) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.ctz + ;; CHECK-NEXT: (global.get $global$1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (global.set $global$1 + ;; CHECK-NEXT: (i32.const 30) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $b + (if + (i32.ctz + (global.get $global$1) + ) + (global.set $global$1 + (i32.const 30) + ) + (block + (global.set $global$1 + (i32.const 100) + ) + ) + ) + ) +) + +;; Outline if control flow, with matching if-condition, if-true, if-false +;; TODO: Ideally outlining would keep the if-true and if-false inline in +;; $outline$, instead of moving them to another outlined function ($outline$_3 +;; & $outline$_4) because of the unique symbol between the if-condition and +;; if-true and the unique symbol between if-true and if-false. +(module + ;; CHECK: (type $0 (func)) + + ;; CHECK: (global $global$1 (mut i32) (i32.const 100)) + (global $global$1 (mut i32) (i32.const 100)) + ;; CHECK: (func $outline$ (type $0) + ;; CHECK-NEXT: (if + ;; CHECK-NEXT: (i32.eqz + ;; CHECK-NEXT: (global.get $global$1) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (call $outline$_3) + ;; CHECK-NEXT: (call $outline$_4) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + + ;; CHECK: (func $outline$_3 (type $0) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 10) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + + ;; CHECK: (func $outline$_4 (type $0) + ;; CHECK-NEXT: (global.set $global$1 + ;; CHECK-NEXT: (i32.const 20) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + + ;; CHECK: (func $a (type $0) + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: ) + (func $a + (if + (i32.eqz + (global.get $global$1) + ) + (drop + (i32.const 10) + ) + (global.set $global$1 + (i32.const 20) + ) + ) + ) + ;; CHECK: (func $b (type $0) + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: ) + (func $b + (if + (i32.eqz + (global.get $global$1) + ) + (drop + (i32.const 10) + ) + (global.set $global$1 + (i32.const 20) + ) + ) + ) +) + ;; Tests that local.get instructions are correctly filtered from being outlined. (module ;; CHECK: (type $0 (func (param i32))) @@ -542,3 +726,40 @@ ;; CHECK-NEXT: (call $outline$) ;; CHECK-NEXT: (call $outline$) ;; CHECK-NEXT: ) + +;; Outline a loop +;; TODO: Ideally, a loop (like any control flow) repeated within a program can +;; be outlined by itself. Right now this is not possible since a control flow +;; is represented by a single symbol and only sequences of symbols >= 2 are +;; candidates for outlining. +(module + ;; CHECK: (type $0 (func)) + + ;; CHECK: (func $outline$ (type $0) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (loop $loop-in + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + + ;; CHECK: (func $a (type $0) + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: ) + (func $a + (drop + (i32.const 0) + ) + (loop (nop)) + ) + ;; CHECK: (func $b (type $0) + ;; CHECK-NEXT: (call $outline$) + ;; CHECK-NEXT: ) + (func $b + (drop + (i32.const 0) + ) + (loop (nop)) + ) +) From c262e5b013c857b1fc167210b196508f5d6b8948 Mon Sep 17 00:00:00 2001 From: Ashley Nelson Date: Thu, 7 Dec 2023 10:22:09 +0000 Subject: [PATCH 2/3] feedback Created using spr 1.3.4 --- test/lit/passes/outlining.wast | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lit/passes/outlining.wast b/test/lit/passes/outlining.wast index be31b31aad9..add389f888d 100644 --- a/test/lit/passes/outlining.wast +++ b/test/lit/passes/outlining.wast @@ -795,7 +795,7 @@ ) ;; Test outlining works with call_indirect -;; 2 results, 0 params, 1 operand +;; 1 results, 0 params, 1 operand (module (table funcref) ;; CHECK: (type $0 (func)) From 55ebcd553c6e8344ee456b3ce2fd1f9b9608b65b Mon Sep 17 00:00:00 2001 From: Ashley Nelson Date: Thu, 7 Dec 2023 10:24:47 +0000 Subject: [PATCH 3/3] singular Created using spr 1.3.4 --- test/lit/passes/outlining.wast | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lit/passes/outlining.wast b/test/lit/passes/outlining.wast index add389f888d..b42ac54ded0 100644 --- a/test/lit/passes/outlining.wast +++ b/test/lit/passes/outlining.wast @@ -795,7 +795,7 @@ ) ;; Test outlining works with call_indirect -;; 1 results, 0 params, 1 operand +;; 1 result, 0 params, 1 operand (module (table funcref) ;; CHECK: (type $0 (func))