Skip to content

Commit 077c531

Browse files
kripkenkripken
committed
fix merge-blocks logic: ensure that optimize() does not change the outside type
1 parent 6303389 commit 077c531

3 files changed

Lines changed: 171 additions & 58 deletions

File tree

‎src/passes/MergeBlocks.cpp‎

Lines changed: 67 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,15 @@ struct BreakValueDropper : public ControlFlowWalker<BreakValueDropper> {
154154
}
155155
};
156156

157+
static bool hasUnreachableChild(Block* block) {
158+
for (auto* test : block->list) {
159+
if (test->type == unreachable) {
160+
return true;
161+
}
162+
}
163+
return false;
164+
}
165+
157166
// core block optimizer routine
158167
static void optimizeBlock(Block* curr, Module* module, PassOptions& passOptions) {
159168
bool more = true;
@@ -168,42 +177,37 @@ static void optimizeBlock(Block* curr, Module* module, PassOptions& passOptions)
168177
if (drop) {
169178
child = drop->value->dynCast<Block>();
170179
if (child) {
171-
// if we move around unreachable code, type changes could occur. avoid that, as
172-
// anyhow it means we should have run dce before getting here
173-
for (auto* test : child->list) {
174-
if (test->type == unreachable) {
180+
if (hasUnreachableChild(child)) {
181+
// don't move around unreachable code, as it can change types
182+
// dce should have been run anyhow
183+
continue;
184+
}
185+
if (child->name.is()) {
186+
Expression* expression = child;
187+
// check if it's ok to remove the value from all breaks to us
188+
ProblemFinder finder(passOptions);
189+
finder.origin = child->name;
190+
finder.walk(expression);
191+
if (finder.found()) {
175192
child = nullptr;
176-
break;
193+
} else {
194+
// fix up breaks
195+
BreakValueDropper fixer(passOptions);
196+
fixer.origin = child->name;
197+
fixer.setModule(module);
198+
fixer.walk(expression);
177199
}
178200
}
179201
if (child) {
180-
if (child->name.is()) {
181-
Expression* expression = child;
182-
// check if it's ok to remove the value from all breaks to us
183-
ProblemFinder finder(passOptions);
184-
finder.origin = child->name;
185-
finder.walk(expression);
186-
if (finder.found()) {
187-
child = nullptr;
188-
} else {
189-
// fix up breaks
190-
BreakValueDropper fixer(passOptions);
191-
fixer.origin = child->name;
192-
fixer.setModule(module);
193-
fixer.walk(expression);
194-
}
195-
}
196-
if (child) {
197-
// we can do it!
198-
// reuse the drop
199-
drop->value = child->list.back();
200-
drop->finalize();
201-
child->list.back() = drop;
202-
child->finalize();
203-
curr->list[i] = child;
204-
more = true;
205-
changed = true;
206-
}
202+
// we can do it!
203+
// reuse the drop
204+
drop->value = child->list.back();
205+
drop->finalize();
206+
child->list.back() = drop;
207+
child->finalize();
208+
curr->list[i] = child;
209+
more = true;
210+
changed = true;
207211
}
208212
}
209213
}
@@ -251,6 +255,23 @@ struct MergeBlocks : public WalkerPass<PostWalker<MergeBlocks>> {
251255
optimizeBlock(curr, getModule(), getPassOptions());
252256
}
253257

258+
// given
259+
// (curr
260+
// (block=child
261+
// (..more..)
262+
// (back)
263+
// )
264+
// (..other..children..)
265+
// )
266+
// if child is a block, we can move this around to
267+
// (block
268+
// (..more..)
269+
// (curr
270+
// (back)
271+
// (..other..children..)
272+
// )
273+
// )
274+
// at which point the block is on the outside and potentially mergeable with an outer block
254275
Block* optimize(Expression* curr, Expression*& child, Block* outer = nullptr, Expression** dependency1 = nullptr, Expression** dependency2 = nullptr) {
255276
if (!child) return outer;
256277
if ((dependency1 && *dependency1) || (dependency2 && *dependency2)) {
@@ -261,18 +282,28 @@ struct MergeBlocks : public WalkerPass<PostWalker<MergeBlocks>> {
261282
}
262283
if (auto* block = child->dynCast<Block>()) {
263284
if (!block->name.is() && block->list.size() >= 2) {
264-
child = block->list.back();
285+
// if we move around unreachable code, type changes could occur. avoid that, as
286+
// anyhow it means we should have run dce before getting here
287+
if (curr->type == none && hasUnreachableChild(block)) {
288+
// moving the block to the outside would replace a none with an unreachable
289+
return outer;
290+
}
291+
auto* back = block->list.back();
292+
if (back->type == unreachable) {
293+
// curr is not reachable, dce could remove it; don't try anything fancy
294+
// here
295+
return outer;
296+
}
297+
child = back;
265298
// we modified child (which is a reference to a pointer), which modifies curr, which might change its type
266299
// (e.g. (drop (block (result i32) .. (unreachable)))
267300
// the child was a block of i32, and is being replaced with an unreachable, so the
268301
// parent will likely need to be unreachable too
269-
auto oldType = curr->type;
270-
ReFinalize().walk(curr);
271302
if (outer == nullptr) {
272303
// reuse the block, move it out
273304
block->list.back() = curr;
274305
// we want the block outside to have the same type as curr had
275-
block->finalize(oldType);
306+
block->finalize(curr->type);
276307
replaceCurrent(block);
277308
return block;
278309
} else {

‎test/passes/remove-unused-names_merge-blocks.txt‎

Lines changed: 64 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,13 @@
166166
(i32.const 20)
167167
)
168168
)
169-
(drop
170-
(i32.const 10)
171-
)
172169
(return
173-
(unreachable)
170+
(block
171+
(drop
172+
(i32.const 10)
173+
)
174+
(unreachable)
175+
)
174176
)
175177
)
176178
(func $binary (type $3)
@@ -297,14 +299,16 @@
297299
)
298300
)
299301
)
300-
(unreachable)
301-
(drop
302-
(i32.const 20)
303-
)
304302
(drop
305-
(i32.add
306-
(i32.const 10)
307-
(i32.const 30)
303+
(block (result i32)
304+
(unreachable)
305+
(drop
306+
(i32.const 20)
307+
)
308+
(i32.add
309+
(i32.const 10)
310+
(i32.const 30)
311+
)
308312
)
309313
)
310314
)
@@ -460,7 +464,7 @@
460464
(drop
461465
(select
462466
(i32.const 20)
463-
(block
467+
(block (result i32)
464468
(unreachable)
465469
(i32.const 40)
466470
)
@@ -478,7 +482,7 @@
478482
(drop
479483
(select
480484
(i32.const 20)
481-
(block
485+
(block (result i32)
482486
(drop
483487
(i32.const 30)
484488
)
@@ -502,7 +506,7 @@
502506
(select
503507
(i32.const 20)
504508
(i32.const 40)
505-
(block
509+
(block (result i32)
506510
(unreachable)
507511
(i32.const 60)
508512
)
@@ -518,7 +522,7 @@
518522
(select
519523
(i32.const 20)
520524
(i32.const 40)
521-
(block
525+
(block (result i32)
522526
(drop
523527
(i32.const 50)
524528
)
@@ -639,7 +643,7 @@
639643
)
640644
(call $call-ii
641645
(i32.const 20)
642-
(block
646+
(block (result i32)
643647
(unreachable)
644648
(i32.const 30)
645649
)
@@ -649,7 +653,7 @@
649653
)
650654
(call $call-ii
651655
(i32.const 20)
652-
(block
656+
(block (result i32)
653657
(drop
654658
(i32.const 30)
655659
)
@@ -825,13 +829,15 @@
825829
)
826830
)
827831
(func $return-different-type (type $4) (result i32)
828-
(drop
829-
(i32.const 2)
830-
)
831832
(drop
832833
(f64.abs
833-
(return
834-
(i32.const 1)
834+
(block
835+
(drop
836+
(i32.const 2)
837+
)
838+
(return
839+
(i32.const 1)
840+
)
835841
)
836842
)
837843
)
@@ -850,4 +856,40 @@
850856
(unreachable)
851857
(f64.const -1)
852858
)
859+
(func $dont-move-unreachable (type $3)
860+
(loop $label$0
861+
(drop
862+
(block (result i32)
863+
(br $label$0)
864+
(i32.const 1)
865+
)
866+
)
867+
)
868+
)
869+
(func $dont-move-unreachable-last (type $3)
870+
(loop $label$0
871+
(drop
872+
(block (result i32)
873+
(call $dont-move-unreachable-last)
874+
(br $label$0)
875+
)
876+
)
877+
)
878+
)
879+
(func $move-around-unreachable-in-middle (type $3)
880+
(loop $label$0
881+
(nop)
882+
(drop
883+
(block $label$3 (result i32)
884+
(drop
885+
(br_if $label$3
886+
(br $label$0)
887+
(i32.const 0)
888+
)
889+
)
890+
(i32.const 1)
891+
)
892+
)
893+
)
894+
)
853895
)

‎test/passes/remove-unused-names_merge-blocks.wast‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,4 +1014,44 @@
10141014
(f64.const -1)
10151015
)
10161016
)
1017+
(func $dont-move-unreachable
1018+
(loop $label$0
1019+
(drop
1020+
(block $label$3 (result i32)
1021+
(br $label$0)
1022+
(i32.const 1)
1023+
)
1024+
)
1025+
)
1026+
)
1027+
(func $dont-move-unreachable-last
1028+
(loop $label$0
1029+
(drop
1030+
(block $label$3 (result i32)
1031+
(call $dont-move-unreachable-last)
1032+
(br $label$0)
1033+
)
1034+
)
1035+
)
1036+
)
1037+
(func $move-around-unreachable-in-middle
1038+
(loop $label$0
1039+
(drop
1040+
(block $label$2 (result i32)
1041+
(block $block2
1042+
(nop)
1043+
)
1044+
(block $label$3 (result i32)
1045+
(drop
1046+
(br_if $label$3
1047+
(br $label$0)
1048+
(i32.const 0)
1049+
)
1050+
)
1051+
(i32.const 1)
1052+
)
1053+
)
1054+
)
1055+
)
1056+
)
10171057
)

0 commit comments

Comments
 (0)