Skip to content

Commit 16a5993

Browse files
tlivelytlively
authored
Revert "Optimize tuple.extract of gets in BinaryInstWriter (#5941)" (#5945)
This reverts commit 56ce1ea. The binary writer optimization is not always correct when stack IR optimizations have run. Revert the change until we can fix it.
1 parent b2e9227 commit 16a5993

File tree

5 files changed

+103
-53
lines changed

5 files changed

+103
-53
lines changed

‎src/wasm-stack.h

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -148,11 +148,6 @@ class BinaryInstWriter : public OverriddenVisitor<BinaryInstWriter> {
148148
InsertOrderedMap<Type, Index> scratchLocals;
149149
void countScratchLocals();
150150
void setScratchLocals();
151-
152-
// local.get, local.tee, and glboal.get expressions that will be followed by
153-
// tuple.extracts. We can optimize these by getting only the local for the
154-
// extracted index.
155-
std::unordered_map<Expression*, Index> extractedGets;
156151
};
157152

158153
// Takes binaryen IR and converts it to something else (binary or stack IR)

‎src/wasm/wasm-stack.cpp

Lines changed: 1 addition & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -87,13 +87,6 @@ void BinaryInstWriter::visitCallIndirect(CallIndirect* curr) {
8787
}
8888

8989
void BinaryInstWriter::visitLocalGet(LocalGet* curr) {
90-
if (auto it = extractedGets.find(curr); it != extractedGets.end()) {
91-
// We have a tuple of locals to get, but we will only end up using one of
92-
// them, so we can just emit that one.
93-
o << int8_t(BinaryConsts::LocalGet)
94-
<< U32LEB(mappedLocals[std::make_pair(curr->index, it->second)]);
95-
return;
96-
}
9790
size_t numValues = func->getLocalType(curr->index).size();
9891
for (Index i = 0; i < numValues; ++i) {
9992
o << int8_t(BinaryConsts::LocalGet)
@@ -103,28 +96,14 @@ void BinaryInstWriter::visitLocalGet(LocalGet* curr) {
10396

10497
void BinaryInstWriter::visitLocalSet(LocalSet* curr) {
10598
size_t numValues = func->getLocalType(curr->index).size();
106-
// If this is a tuple, set all the elements with nonzero index.
10799
for (Index i = numValues - 1; i >= 1; --i) {
108100
o << int8_t(BinaryConsts::LocalSet)
109101
<< U32LEB(mappedLocals[std::make_pair(curr->index, i)]);
110102
}
111103
if (!curr->isTee()) {
112-
// This is not a tee, so just finish setting the values.
113104
o << int8_t(BinaryConsts::LocalSet)
114105
<< U32LEB(mappedLocals[std::make_pair(curr->index, 0)]);
115-
} else if (auto it = extractedGets.find(curr); it != extractedGets.end()) {
116-
// We only need to get the single extracted value.
117-
if (it->second == 0) {
118-
o << int8_t(BinaryConsts::LocalTee)
119-
<< U32LEB(mappedLocals[std::make_pair(curr->index, 0)]);
120-
} else {
121-
o << int8_t(BinaryConsts::LocalSet)
122-
<< U32LEB(mappedLocals[std::make_pair(curr->index, 0)]);
123-
o << int8_t(BinaryConsts::LocalGet)
124-
<< U32LEB(mappedLocals[std::make_pair(curr->index, it->second)]);
125-
}
126106
} else {
127-
// We need to get all the values.
128107
o << int8_t(BinaryConsts::LocalTee)
129108
<< U32LEB(mappedLocals[std::make_pair(curr->index, 0)]);
130109
for (Index i = 1; i < numValues; ++i) {
@@ -135,14 +114,8 @@ void BinaryInstWriter::visitLocalSet(LocalSet* curr) {
135114
}
136115

137116
void BinaryInstWriter::visitGlobalGet(GlobalGet* curr) {
138-
Index index = parent.getGlobalIndex(curr->name);
139-
if (auto it = extractedGets.find(curr); it != extractedGets.end()) {
140-
// We have a tuple of globals to get, but we will only end up using one of
141-
// them, so we can just emit that one.
142-
o << int8_t(BinaryConsts::GlobalGet) << U32LEB(index + it->second);
143-
return;
144-
}
145117
// Emit a global.get for each element if this is a tuple global
118+
Index index = parent.getGlobalIndex(curr->name);
146119
size_t numValues = curr->type.size();
147120
for (Index i = 0; i < numValues; ++i) {
148121
o << int8_t(BinaryConsts::GlobalGet) << U32LEB(index + i);
@@ -1997,10 +1970,6 @@ void BinaryInstWriter::visitTupleMake(TupleMake* curr) {
19971970
}
19981971

19991972
void BinaryInstWriter::visitTupleExtract(TupleExtract* curr) {
2000-
if (extractedGets.count(curr->tuple)) {
2001-
// We already have just the extracted value on the stack.
2002-
return;
2003-
}
20041973
size_t numVals = curr->tuple->type.size();
20051974
// Drop all values after the one we want
20061975
for (size_t i = curr->index + 1; i < numVals; ++i) {
@@ -2537,7 +2506,6 @@ void BinaryInstWriter::mapLocalsAndEmitHeader() {
25372506
}
25382507
}
25392508
setScratchLocals();
2540-
25412509
o << U32LEB(numLocalsByType.size());
25422510
for (auto& localType : localTypes) {
25432511
o << U32LEB(numLocalsByType.at(localType));
@@ -2564,15 +2532,6 @@ void BinaryInstWriter::countScratchLocals() {
25642532
for (auto& [type, _] : scratchLocals) {
25652533
noteLocalType(type);
25662534
}
2567-
// While we have all the tuple.extracts, also find extracts of local.gets,
2568-
// local.tees, and global.gets that we can optimize.
2569-
for (auto* extract : extracts.list) {
2570-
auto* tuple = extract->tuple;
2571-
if (tuple->is<LocalGet>() || tuple->is<LocalSet>() ||
2572-
tuple->is<GlobalGet>()) {
2573-
extractedGets.insert({tuple, extract->index});
2574-
}
2575-
}
25762535
}
25772536

25782537
void BinaryInstWriter::setScratchLocals() {

‎test/exception-handling.wast.fromBinary

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
(local $1 i64)
2121
(local $2 (i32 i64))
2222
(local $3 i32)
23+
(local $4 i32)
2324
(try $label$3
2425
(do
2526
(throw $e-i32
@@ -59,7 +60,15 @@
5960
)
6061
)
6162
(drop
62-
(local.get $x)
63+
(block (result i32)
64+
(local.set $4
65+
(local.get $x)
66+
)
67+
(drop
68+
(local.get $1)
69+
)
70+
(local.get $4)
71+
)
6372
)
6473
)
6574
)

‎test/exception-handling.wast.fromBinary.noDebugInfo

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
(local $1 i64)
2121
(local $2 (i32 i64))
2222
(local $3 i32)
23+
(local $4 i32)
2324
(try $label$3
2425
(do
2526
(throw $tag$0
@@ -59,7 +60,15 @@
5960
)
6061
)
6162
(drop
62-
(local.get $0)
63+
(block (result i32)
64+
(local.set $4
65+
(local.get $0)
66+
)
67+
(drop
68+
(local.get $1)
69+
)
70+
(local.get $4)
71+
)
6372
)
6473
)
6574
)

‎test/lit/multivalue.wast

Lines changed: 82 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,13 @@
155155
;; CHECK-NEXT: (local $5 (i32 i64 f32))
156156
;; CHECK-NEXT: (local $6 i64)
157157
;; CHECK-NEXT: (local $7 i32)
158+
;; CHECK-NEXT: (local $8 i64)
159+
;; CHECK-NEXT: (local $9 i32)
160+
;; CHECK-NEXT: (local $10 i64)
161+
;; CHECK-NEXT: (local $11 i32)
162+
;; CHECK-NEXT: (local $12 i64)
163+
;; CHECK-NEXT: (local $13 i32)
164+
;; CHECK-NEXT: (local $14 f32)
158165
;; CHECK-NEXT: (local.set $5
159166
;; CHECK-NEXT: (call $triple)
160167
;; CHECK-NEXT: )
@@ -183,10 +190,69 @@
183190
;; CHECK-NEXT: (local.get $7)
184191
;; CHECK-NEXT: )
185192
;; CHECK-NEXT: )
193+
;; CHECK-NEXT: (drop
194+
;; CHECK-NEXT: (block (result i32)
195+
;; CHECK-NEXT: (local.set $9
196+
;; CHECK-NEXT: (local.get $x)
197+
;; CHECK-NEXT: )
198+
;; CHECK-NEXT: (drop
199+
;; CHECK-NEXT: (block (result i64)
200+
;; CHECK-NEXT: (local.set $8
201+
;; CHECK-NEXT: (local.get $1)
202+
;; CHECK-NEXT: )
203+
;; CHECK-NEXT: (local.set $4
204+
;; CHECK-NEXT: (local.get $3)
205+
;; CHECK-NEXT: )
206+
;; CHECK-NEXT: (local.get $8)
207+
;; CHECK-NEXT: )
208+
;; CHECK-NEXT: )
209+
;; CHECK-NEXT: (local.get $9)
210+
;; CHECK-NEXT: )
211+
;; CHECK-NEXT: )
186212
;; CHECK-NEXT: (tuple.make
187-
;; CHECK-NEXT: (local.get $3)
188-
;; CHECK-NEXT: (local.get $1)
189-
;; CHECK-NEXT: (local.get $x)
213+
;; CHECK-NEXT: (block (result f32)
214+
;; CHECK-NEXT: (local.set $14
215+
;; CHECK-NEXT: (local.get $4)
216+
;; CHECK-NEXT: )
217+
;; CHECK-NEXT: (drop
218+
;; CHECK-NEXT: (block (result i32)
219+
;; CHECK-NEXT: (local.set $11
220+
;; CHECK-NEXT: (local.get $x)
221+
;; CHECK-NEXT: )
222+
;; CHECK-NEXT: (local.set $2
223+
;; CHECK-NEXT: (block (result i64)
224+
;; CHECK-NEXT: (local.set $10
225+
;; CHECK-NEXT: (local.get $1)
226+
;; CHECK-NEXT: )
227+
;; CHECK-NEXT: (drop
228+
;; CHECK-NEXT: (local.get $3)
229+
;; CHECK-NEXT: )
230+
;; CHECK-NEXT: (local.get $10)
231+
;; CHECK-NEXT: )
232+
;; CHECK-NEXT: )
233+
;; CHECK-NEXT: (local.get $11)
234+
;; CHECK-NEXT: )
235+
;; CHECK-NEXT: )
236+
;; CHECK-NEXT: (local.get $14)
237+
;; CHECK-NEXT: )
238+
;; CHECK-NEXT: (local.get $2)
239+
;; CHECK-NEXT: (block (result i32)
240+
;; CHECK-NEXT: (local.set $13
241+
;; CHECK-NEXT: (local.get $x)
242+
;; CHECK-NEXT: )
243+
;; CHECK-NEXT: (drop
244+
;; CHECK-NEXT: (block (result i64)
245+
;; CHECK-NEXT: (local.set $12
246+
;; CHECK-NEXT: (local.get $1)
247+
;; CHECK-NEXT: )
248+
;; CHECK-NEXT: (drop
249+
;; CHECK-NEXT: (local.get $3)
250+
;; CHECK-NEXT: )
251+
;; CHECK-NEXT: (local.get $12)
252+
;; CHECK-NEXT: )
253+
;; CHECK-NEXT: )
254+
;; CHECK-NEXT: (local.get $13)
255+
;; CHECK-NEXT: )
190256
;; CHECK-NEXT: )
191257
;; CHECK-NEXT: )
192258
(func $reverse (result f32 i64 i32)
@@ -230,6 +296,7 @@
230296
;; CHECK: (func $global (type $0) (result i32 i64)
231297
;; CHECK-NEXT: (local $0 i64)
232298
;; CHECK-NEXT: (local $1 i32)
299+
;; CHECK-NEXT: (local $2 i32)
233300
;; CHECK-NEXT: (global.set $g1
234301
;; CHECK-NEXT: (block (result i32)
235302
;; CHECK-NEXT: (local.set $1
@@ -242,7 +309,18 @@
242309
;; CHECK-NEXT: )
243310
;; CHECK-NEXT: )
244311
;; CHECK-NEXT: (drop
245-
;; CHECK-NEXT: (global.get $g2)
312+
;; CHECK-NEXT: (block (result i32)
313+
;; CHECK-NEXT: (local.set $2
314+
;; CHECK-NEXT: (global.get $g1)
315+
;; CHECK-NEXT: )
316+
;; CHECK-NEXT: (local.set $0
317+
;; CHECK-NEXT: (global.get $g2)
318+
;; CHECK-NEXT: )
319+
;; CHECK-NEXT: (local.get $2)
320+
;; CHECK-NEXT: )
321+
;; CHECK-NEXT: )
322+
;; CHECK-NEXT: (drop
323+
;; CHECK-NEXT: (local.get $0)
246324
;; CHECK-NEXT: )
247325
;; CHECK-NEXT: (tuple.make
248326
;; CHECK-NEXT: (global.get $global$2)

0 commit comments

Comments
 (0)