From 86b2508e537b99e88ad6fe34a7ddd3a517fb1482 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 6 Dec 2023 20:42:03 -0800 Subject: [PATCH 1/7] Preserve multivalue drops in IRBuilder In Binaryen IR, we allow single `Drop` expressions to drop multiple values packaged up as a tuple. When using IRBuilder to rebuild IR containing such a drop, it previously treated the drop as a normal WebAssembly drop that dropped only a single value, producing invalid IR that had extra, undropped values. Fix the problem by preserving the arity of `Drop` inputs in IRBuilder. To avoid bloating the IR, thread the size of the desired value through IRBuilder's pop implementation so that tuple values do not need to be split up and recombined. --- src/wasm-ir-builder.h | 11 ++++++++--- src/wasm/wasm-ir-builder.cpp | 38 ++++++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/wasm-ir-builder.h b/src/wasm-ir-builder.h index 8b01977be3a..70f65e07fb5 100644 --- a/src/wasm-ir-builder.h +++ b/src/wasm-ir-builder.h @@ -200,6 +200,7 @@ class IRBuilder : public UnifiedExpressionVisitor> { // Private functions that must be public for technical reasons. [[nodiscard]] Result<> visitExpression(Expression*); + [[nodiscard]] Result<> visitDrop(Drop*); [[nodiscard]] Result<> visitIf(If*); [[nodiscard]] Result<> visitReturn(Return*); [[nodiscard]] Result<> visitStructNew(StructNew*); @@ -463,7 +464,7 @@ class IRBuilder : public UnifiedExpressionVisitor> { [[nodiscard]] Result getLabelName(Index label); [[nodiscard]] Result getDelegateLabelName(Index label); [[nodiscard]] Result addScratchLocal(Type); - [[nodiscard]] Result pop(); + [[nodiscard]] Result pop(size_t size = 1); struct HoistedVal { // The index in the stack of the original value-producing expression. @@ -478,8 +479,12 @@ class IRBuilder : public UnifiedExpressionVisitor> { // Transform the stack as necessary such that the original producer of the // hoisted value will be popped along with the final expression that produces // the value, if they are different. May only be called directly after - // hoistLastValue(). - [[nodiscard]] Result<> packageHoistedValue(const HoistedVal&); + // hoistLastValue(). `sizeHint` is the size of the type we ultimately want to + // consume, so if the hoisted values has `sizeHint` elements, it is left + // intact even if it is a tuple. Otherwise, hoisted tuple values will be + // broken into pieces. + [[nodiscard]] Result<> packageHoistedValue(const HoistedVal&, + size_t sizeHint = 1); [[nodiscard]] Result getBranchValue(Name labelName, std::optional label); diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index d3f48b1c022..e1c1df404f4 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -90,7 +90,8 @@ MaybeResult IRBuilder::hoistLastValue() { return HoistedVal{Index(index), get}; } -Result<> IRBuilder::packageHoistedValue(const HoistedVal& hoisted) { +Result<> IRBuilder::packageHoistedValue(const HoistedVal& hoisted, + size_t sizeHint) { auto& scope = getScope(); assert(!scope.exprStack.empty()); @@ -106,7 +107,7 @@ Result<> IRBuilder::packageHoistedValue(const HoistedVal& hoisted) { auto type = scope.exprStack.back()->type; - if (!type.isTuple()) { + if (type.size() == sizeHint) { if (hoisted.get) { packageAsBlock(type); } @@ -158,7 +159,8 @@ void IRBuilder::push(Expression* expr) { DBG(dump()); } -Result IRBuilder::pop() { +Result IRBuilder::pop(size_t size) { + assert(size >= 1); auto& scope = getScope(); // Find the suffix of expressions that do not produce values. @@ -173,11 +175,25 @@ Result IRBuilder::pop() { return Err{"popping from empty stack"}; } - CHECK_ERR(packageHoistedValue(*hoisted)); + CHECK_ERR(packageHoistedValue(*hoisted, size)); auto* ret = scope.exprStack.back(); - scope.exprStack.pop_back(); - return ret; + if (ret->type.size() == size) { + scope.exprStack.pop_back(); + return ret; + } + + // The last value-producing expression did not produce exactly the right + // number of values, so we need to construct a tuple piecewise instead. + assert(size > 1); + std::vector elems; + elems.resize(size); + for (int i = size - 1; i >= 0; --i) { + auto elem = pop(); + CHECK_ERR(elem); + elems[i] = *elem; + } + return builder.makeTupleMake(elems); } Result IRBuilder::build() { @@ -319,6 +335,16 @@ Result<> IRBuilder::visitExpression(Expression* curr) { return Ok{}; } +Result<> IRBuilder::visitDrop(Drop* curr) { + // Multivalue drops must remain multivalue drops. + if (curr->value->type.isTuple()) { + auto val = pop(curr->value->type.size()); + CHECK_ERR(val); + curr->value = *val; + } + return visitExpression(curr); +} + Result<> IRBuilder::visitIf(If* curr) { // Only the condition is popped from the stack. The ifTrue and ifFalse are // self-contained so we do not modify them. From 066426b40054ea1fd82b1a31d5fb453439c8105e Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 7 Dec 2023 10:29:20 -0800 Subject: [PATCH 2/7] fix --- src/wasm/wasm-ir-builder.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index e1c1df404f4..7efef859c7e 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -341,6 +341,7 @@ Result<> IRBuilder::visitDrop(Drop* curr) { auto val = pop(curr->value->type.size()); CHECK_ERR(val); curr->value = *val; + return Ok{}; } return visitExpression(curr); } From 1451c348f5cd32873d7a704cac9267092e105677 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 7 Dec 2023 11:31:14 -0800 Subject: [PATCH 3/7] test --- test/lit/passes/outlining.wast | 46 +++++++++++++++++----------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/test/lit/passes/outlining.wast b/test/lit/passes/outlining.wast index 46a3de0bd36..960a231c83f 100644 --- a/test/lit/passes/outlining.wast +++ b/test/lit/passes/outlining.wast @@ -729,20 +729,16 @@ ) ;; Test outlining works with call_indirect -;; 0 results, 2 params, 3 operands +;; 0 results, 0 params, 1 operand (module (table funcref) ;; CHECK: (type $0 (func)) - ;; CHECK: (type $1 (func (param i32 i32))) - ;; CHECK: (table $0 0 funcref) ;; CHECK: (func $outline$ (type $0) - ;; CHECK-NEXT: (call_indirect $0 (type $1) + ;; CHECK-NEXT: (call_indirect $0 (type $0) ;; CHECK-NEXT: (i32.const 0) - ;; CHECK-NEXT: (i32.const 1) - ;; CHECK-NEXT: (i32.const 2) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -752,31 +748,29 @@ ;; CHECK-NEXT: ) (func $a (call_indirect - (param i32 i32) (i32.const 0) - (i32.const 1) - (i32.const 2) ) (call_indirect - (param i32 i32) (i32.const 0) - (i32.const 1) - (i32.const 2) ) ) ) ;; Test outlining works with call_indirect -;; 0 results, 0 params, 1 operand +;; 1 result, 0 params, 1 operand (module (table funcref) ;; CHECK: (type $0 (func)) + ;; CHECK: (type $1 (func (result i32))) + ;; CHECK: (table $0 0 funcref) ;; CHECK: (func $outline$ (type $0) - ;; CHECK-NEXT: (call_indirect $0 (type $0) - ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (call_indirect $0 (type $1) + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) @@ -785,22 +779,28 @@ ;; CHECK-NEXT: (call $outline$) ;; CHECK-NEXT: ) (func $a - (call_indirect - (i32.const 0) + (drop + (call_indirect + (result i32) + (i32.const 0) + ) ) - (call_indirect - (i32.const 0) + (drop + (call_indirect + (result i32) + (i32.const 0) + ) ) ) ) ;; Test outlining works with call_indirect -;; 1 result, 0 params, 1 operand +;; 2 results, 0 params, 1 operand (module (table funcref) ;; CHECK: (type $0 (func)) - ;; CHECK: (type $1 (func (result i32))) + ;; CHECK: (type $1 (func (result i32 i32))) ;; CHECK: (table $0 0 funcref) @@ -819,13 +819,13 @@ (func $a (drop (call_indirect - (result i32) + (result i32 i32) (i32.const 0) ) ) (drop (call_indirect - (result i32) + (result i32 i32) (i32.const 0) ) ) From 19a89adbc30620d2177de850345acd8eab1a8b8f Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 12 Dec 2023 19:22:24 -0800 Subject: [PATCH 4/7] update tests --- test/lit/passes/outlining.wast | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/lit/passes/outlining.wast b/test/lit/passes/outlining.wast index 960a231c83f..1d7d0e6c2e7 100644 --- a/test/lit/passes/outlining.wast +++ b/test/lit/passes/outlining.wast @@ -805,7 +805,7 @@ ;; CHECK: (table $0 0 funcref) ;; CHECK: (func $outline$ (type $0) - ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (tuple.drop 2 ;; CHECK-NEXT: (call_indirect $0 (type $1) ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) @@ -817,13 +817,13 @@ ;; CHECK-NEXT: (call $outline$) ;; CHECK-NEXT: ) (func $a - (drop + (tuple.drop 2 (call_indirect (result i32 i32) (i32.const 0) ) ) - (drop + (tuple.drop 2 (call_indirect (result i32 i32) (i32.const 0) From 8758620f8e7d489bac06168389cd6fbf607144a3 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 13 Dec 2023 12:10:42 -0800 Subject: [PATCH 5/7] fix memory error --- src/wasm-ir-builder.h | 3 ++- src/wasm/wasm-ir-builder.cpp | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/wasm-ir-builder.h b/src/wasm-ir-builder.h index 70f65e07fb5..897c15cf7f3 100644 --- a/src/wasm-ir-builder.h +++ b/src/wasm-ir-builder.h @@ -200,7 +200,8 @@ class IRBuilder : public UnifiedExpressionVisitor> { // Private functions that must be public for technical reasons. [[nodiscard]] Result<> visitExpression(Expression*); - [[nodiscard]] Result<> visitDrop(Drop*); + [[nodiscard]] Result<> + visitDrop(Drop*, std::optional arith = std::nullopt); [[nodiscard]] Result<> visitIf(If*); [[nodiscard]] Result<> visitReturn(Return*); [[nodiscard]] Result<> visitStructNew(StructNew*); diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 7efef859c7e..55a63a91654 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -335,9 +335,12 @@ Result<> IRBuilder::visitExpression(Expression* curr) { return Ok{}; } -Result<> IRBuilder::visitDrop(Drop* curr) { +Result<> IRBuilder::visitDrop(Drop* curr, std::optional arity) { // Multivalue drops must remain multivalue drops. - if (curr->value->type.isTuple()) { + if (!arity) { + arity = curr->value->type.size(); + } + if (*arity >= 2) { auto val = pop(curr->value->type.size()); CHECK_ERR(val); curr->value = *val; @@ -1210,7 +1213,7 @@ Result<> IRBuilder::makeSelect(std::optional type) { Result<> IRBuilder::makeDrop() { Drop curr; - CHECK_ERR(visitDrop(&curr)); + CHECK_ERR(visitDrop(&curr, 1)); push(builder.makeDrop(curr.value)); return Ok{}; } From 257a807b0310bea021dc48976b22a364d32fea38 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 13 Dec 2023 12:12:44 -0800 Subject: [PATCH 6/7] fix --- src/wasm-ir-builder.h | 2 +- src/wasm/wasm-ir-builder.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wasm-ir-builder.h b/src/wasm-ir-builder.h index 897c15cf7f3..53ecea16721 100644 --- a/src/wasm-ir-builder.h +++ b/src/wasm-ir-builder.h @@ -201,7 +201,7 @@ class IRBuilder : public UnifiedExpressionVisitor> { // Private functions that must be public for technical reasons. [[nodiscard]] Result<> visitExpression(Expression*); [[nodiscard]] Result<> - visitDrop(Drop*, std::optional arith = std::nullopt); + visitDrop(Drop*, std::optional arity = std::nullopt); [[nodiscard]] Result<> visitIf(If*); [[nodiscard]] Result<> visitReturn(Return*); [[nodiscard]] Result<> visitStructNew(StructNew*); diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 55a63a91654..fbb116aa993 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -341,7 +341,7 @@ Result<> IRBuilder::visitDrop(Drop* curr, std::optional arity) { arity = curr->value->type.size(); } if (*arity >= 2) { - auto val = pop(curr->value->type.size()); + auto val = pop(*arity); CHECK_ERR(val); curr->value = *val; return Ok{}; From 9ab99a7f0731cd98e3dc1458d1e8d0e6b2a2ac6a Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 13 Dec 2023 12:22:46 -0800 Subject: [PATCH 7/7] thank you ashley for the suggestion --- src/wasm-ir-builder.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wasm-ir-builder.h b/src/wasm-ir-builder.h index 53ecea16721..e1d2ce1fd2b 100644 --- a/src/wasm-ir-builder.h +++ b/src/wasm-ir-builder.h @@ -481,9 +481,9 @@ class IRBuilder : public UnifiedExpressionVisitor> { // hoisted value will be popped along with the final expression that produces // the value, if they are different. May only be called directly after // hoistLastValue(). `sizeHint` is the size of the type we ultimately want to - // consume, so if the hoisted values has `sizeHint` elements, it is left - // intact even if it is a tuple. Otherwise, hoisted tuple values will be - // broken into pieces. + // consume, so if the hoisted value has `sizeHint` elements, it is left intact + // even if it is a tuple. Otherwise, hoisted tuple values will be broken into + // pieces. [[nodiscard]] Result<> packageHoistedValue(const HoistedVal&, size_t sizeHint = 1);