Skip to content

Commit 6f0f2e0

Browse files
tlivelytlively
authored
Make more Ifs unreachable (#7094)
Previously the only Ifs that were typed unreachable were those in which both arms were unreachable and those in which the condition was unreachable that would have otherwise been typed none. This caused problems in IRBuilder because Ifs with unreachable conditions and value-returning arms would have concrete types, effectively hiding the unreachable condition from the logic for dropping concretely typed expressions preceding an unreachable expression when finishing a scope. Relax the conditions under which an If can be typed unreachable so that all Ifs with unreachable conditions or two unreachable arms are typed unreachable. Propagating unreachability more eagerly this way makes various optimizations of Ifs more powerful. It also requires new handling for unreachable Ifs with concretely typed arms in the Printer to ensure that printed wat remains valid. Also update Unsubtyping, Flatten, and CodeFolding to account for the newly unreachable Ifs.
1 parent cd3805e commit 6f0f2e0

19 files changed

Lines changed: 282 additions & 108 deletions

‎src/ir/subtype-exprs.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ struct SubtypingDiscoverer : public OverriddenVisitor<SubType> {
122122
}
123123
}
124124
void visitIf(If* curr) {
125-
if (curr->ifFalse) {
125+
if (curr->ifFalse && curr->type != Type::unreachable) {
126126
self()->noteSubtype(curr->ifTrue, curr);
127127
self()->noteSubtype(curr->ifFalse, curr);
128128
}

‎src/passes/CodeFolding.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,13 @@ struct CodeFolding
234234
if (!curr->ifFalse) {
235235
return;
236236
}
237+
if (curr->condition->type == Type::unreachable) {
238+
// If the arms are foldable and concrete, we would be replacing an
239+
// unreachable If with a concrete block, which may or may not be valid,
240+
// depending on the context. Leave this for DCE rather than trying to
241+
// handle that.
242+
return;
243+
}
237244
// If both are blocks, look for a tail we can merge.
238245
auto* left = curr->ifTrue->dynCast<Block>();
239246
auto* right = curr->ifFalse->dynCast<Block>();

‎src/passes/Flatten.cpp‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -147,20 +147,24 @@ struct Flatten
147147
// arm preludes go in the arms. we must also remove an if value
148148
auto* originalIfTrue = iff->ifTrue;
149149
auto* originalIfFalse = iff->ifFalse;
150-
auto type = iff->type;
150+
auto type = iff->ifFalse ? Type::getLeastUpperBound(iff->ifTrue->type,
151+
iff->ifFalse->type)
152+
: Type::none;
151153
Expression* prelude = nullptr;
152154
if (type.isConcrete()) {
153155
Index temp = builder.addVar(getFunction(), type);
154156
if (iff->ifTrue->type.isConcrete()) {
155157
iff->ifTrue = builder.makeLocalSet(temp, iff->ifTrue);
156158
}
157-
if (iff->ifFalse && iff->ifFalse->type.isConcrete()) {
159+
if (iff->ifFalse->type.isConcrete()) {
158160
iff->ifFalse = builder.makeLocalSet(temp, iff->ifFalse);
159161
}
160-
// the whole if (+any preludes from the condition) is now a prelude
161-
prelude = rep;
162-
// and we leave just a get of the value
163-
rep = builder.makeLocalGet(temp, type);
162+
if (curr->type.isConcrete()) {
163+
// the whole if (+any preludes from the condition) is now a prelude
164+
prelude = rep;
165+
// and we leave just a get of the value
166+
rep = builder.makeLocalGet(temp, type);
167+
}
164168
}
165169
iff->ifTrue = getPreludesWithExpression(originalIfTrue, iff->ifTrue);
166170
if (iff->ifFalse) {

‎src/passes/Print.cpp‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -460,9 +460,16 @@ struct PrintExpressionContents
460460
}
461461
void visitIf(If* curr) {
462462
printMedium(o, "if");
463-
if (curr->type.isConcrete()) {
463+
// Ifs are unreachable if their condition is unreachable, but in that case
464+
// the arms might have some concrete type we have to account for to produce
465+
// valid wat.
466+
auto type = curr->type;
467+
if (curr->condition->type == Type::unreachable && curr->ifFalse) {
468+
type = Type::getLeastUpperBound(curr->ifTrue->type, curr->ifFalse->type);
469+
}
470+
if (type.isConcrete()) {
464471
o << ' ';
465-
printBlockType(Signature(Type::none, curr->type));
472+
printBlockType(Signature(Type::none, type));
466473
}
467474
}
468475
void visitLoop(Loop* curr) {

‎src/wasm/wasm-validator.cpp‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -865,7 +865,16 @@ void FunctionValidator::visitIf(If* curr) {
865865
curr,
866866
"returning if-else's false must have right type");
867867
} else {
868-
if (curr->condition->type != Type::unreachable) {
868+
if (curr->condition->type == Type::unreachable) {
869+
shouldBeTrue(
870+
curr->ifTrue->type == Type::unreachable ||
871+
curr->ifFalse->type == Type::unreachable ||
872+
(curr->ifTrue->type == Type::none &&
873+
curr->ifFalse->type == Type::none) ||
874+
Type::hasLeastUpperBound(curr->ifTrue->type, curr->ifFalse->type),
875+
curr,
876+
"arms of unreachable if-else must have compatible types");
877+
} else {
869878
shouldBeEqual(curr->ifTrue->type,
870879
Type(Type::unreachable),
871880
curr,
@@ -876,18 +885,6 @@ void FunctionValidator::visitIf(If* curr) {
876885
"unreachable if-else must have unreachable false");
877886
}
878887
}
879-
if (curr->ifTrue->type.isConcrete()) {
880-
shouldBeSubType(curr->ifTrue->type,
881-
curr->type,
882-
curr,
883-
"if type must match concrete ifTrue");
884-
}
885-
if (curr->ifFalse->type.isConcrete()) {
886-
shouldBeSubType(curr->ifFalse->type,
887-
curr->type,
888-
curr,
889-
"if type must match concrete ifFalse");
890-
}
891888
}
892889
}
893890

‎src/wasm/wasm.cpp‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -215,28 +215,20 @@ void Block::finalize(std::optional<Type> type_, Breakability breakability) {
215215
}
216216

217217
void If::finalize(std::optional<Type> type_) {
218-
if (type_) {
219-
type = *type_;
220-
if (type == Type::none && (condition->type == Type::unreachable ||
221-
(ifFalse && ifTrue->type == Type::unreachable &&
222-
ifFalse->type == Type::unreachable))) {
223-
type = Type::unreachable;
224-
}
218+
// The If is unreachable if the condition is unreachable or both arms are
219+
// unreachable.
220+
if (condition->type == Type::unreachable ||
221+
(ifFalse && ifTrue->type == Type::unreachable &&
222+
ifFalse->type == Type::unreachable)) {
223+
type = Type::unreachable;
225224
return;
226225
}
227226

228-
type = ifFalse ? Type::getLeastUpperBound(ifTrue->type, ifFalse->type)
229-
: Type::none;
230-
// if the arms return a value, leave it even if the condition
231-
// is unreachable, we still mark ourselves as having that type, e.g.
232-
// (if (result i32)
233-
// (unreachable)
234-
// (i32.const 10)
235-
// (i32.const 20)
236-
// )
237-
// otherwise, if the condition is unreachable, so is the if
238-
if (type == Type::none && condition->type == Type::unreachable) {
239-
type = Type::unreachable;
227+
if (type_) {
228+
type = *type_;
229+
} else {
230+
type = ifFalse ? Type::getLeastUpperBound(ifTrue->type, ifFalse->type)
231+
: Type::none;
240232
}
241233
}
242234

‎test/lit/passes/code-folding.wast‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -743,12 +743,20 @@
743743
)
744744

745745
;; CHECK: (func $unreachable-if (type $0)
746-
;; CHECK-NEXT: (drop
746+
;; CHECK-NEXT: (if
747747
;; CHECK-NEXT: (unreachable)
748-
;; CHECK-NEXT: )
749-
;; CHECK-NEXT: (nop)
750-
;; CHECK-NEXT: (drop
751-
;; CHECK-NEXT: (i32.const 1)
748+
;; CHECK-NEXT: (then
749+
;; CHECK-NEXT: (nop)
750+
;; CHECK-NEXT: (drop
751+
;; CHECK-NEXT: (i32.const 1)
752+
;; CHECK-NEXT: )
753+
;; CHECK-NEXT: )
754+
;; CHECK-NEXT: (else
755+
;; CHECK-NEXT: (nop)
756+
;; CHECK-NEXT: (drop
757+
;; CHECK-NEXT: (i32.const 1)
758+
;; CHECK-NEXT: )
759+
;; CHECK-NEXT: )
752760
;; CHECK-NEXT: )
753761
;; CHECK-NEXT: )
754762
(func $unreachable-if
@@ -773,14 +781,17 @@
773781
;; CHECK-NEXT: (if
774782
;; CHECK-NEXT: (unreachable)
775783
;; CHECK-NEXT: (then
784+
;; CHECK-NEXT: (drop
785+
;; CHECK-NEXT: (i32.const 1)
786+
;; CHECK-NEXT: )
776787
;; CHECK-NEXT: )
777788
;; CHECK-NEXT: (else
778789
;; CHECK-NEXT: (nop)
790+
;; CHECK-NEXT: (drop
791+
;; CHECK-NEXT: (i32.const 1)
792+
;; CHECK-NEXT: )
779793
;; CHECK-NEXT: )
780794
;; CHECK-NEXT: )
781-
;; CHECK-NEXT: (drop
782-
;; CHECK-NEXT: (i32.const 1)
783-
;; CHECK-NEXT: )
784795
;; CHECK-NEXT: )
785796
(func $unreachable-if-suffix
786797
(if
@@ -800,16 +811,13 @@
800811
)
801812

802813
;; CHECK: (func $unreachable-if-concrete-arms (type $0)
803-
;; CHECK-NEXT: (drop
804-
;; CHECK-NEXT: (block (result i32)
805-
;; CHECK-NEXT: (if
806-
;; CHECK-NEXT: (unreachable)
807-
;; CHECK-NEXT: (then
808-
;; CHECK-NEXT: )
809-
;; CHECK-NEXT: (else
810-
;; CHECK-NEXT: (nop)
811-
;; CHECK-NEXT: )
812-
;; CHECK-NEXT: )
814+
;; CHECK-NEXT: (if (result i32)
815+
;; CHECK-NEXT: (unreachable)
816+
;; CHECK-NEXT: (then
817+
;; CHECK-NEXT: (i32.const 1)
818+
;; CHECK-NEXT: )
819+
;; CHECK-NEXT: (else
820+
;; CHECK-NEXT: (nop)
813821
;; CHECK-NEXT: (i32.const 1)
814822
;; CHECK-NEXT: )
815823
;; CHECK-NEXT: )

‎test/lit/passes/flatten_all-features.wast‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,6 @@
620620
;; CHECK-NEXT: (local $1 i32)
621621
;; CHECK-NEXT: (local $2 i32)
622622
;; CHECK-NEXT: (local $3 i32)
623-
;; CHECK-NEXT: (local $4 i32)
624623
;; CHECK-NEXT: (block $x
625624
;; CHECK-NEXT: (block
626625
;; CHECK-NEXT: (local.set $0
@@ -646,18 +645,13 @@
646645
;; CHECK-NEXT: )
647646
;; CHECK-NEXT: )
648647
;; CHECK-NEXT: )
649-
;; CHECK-NEXT: (local.set $3
650-
;; CHECK-NEXT: (local.get $2)
651-
;; CHECK-NEXT: )
652-
;; CHECK-NEXT: (local.set $1
653-
;; CHECK-NEXT: (local.get $3)
654-
;; CHECK-NEXT: )
648+
;; CHECK-NEXT: (unreachable)
655649
;; CHECK-NEXT: )
656-
;; CHECK-NEXT: (local.set $4
650+
;; CHECK-NEXT: (local.set $3
657651
;; CHECK-NEXT: (local.get $1)
658652
;; CHECK-NEXT: )
659653
;; CHECK-NEXT: (return
660-
;; CHECK-NEXT: (local.get $4)
654+
;; CHECK-NEXT: (local.get $3)
661655
;; CHECK-NEXT: )
662656
;; CHECK-NEXT: )
663657
(func $a13 (result i32)

‎test/lit/passes/optimize-instructions-ignore-traps.wast‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@
213213
;; CHECK-NEXT: (i32.const 1)
214214
;; CHECK-NEXT: )
215215
;; CHECK-NEXT: )
216-
;; CHECK-NEXT: (local.set $0
216+
;; CHECK-NEXT: (local.tee $0
217217
;; CHECK-NEXT: (if (result i32)
218218
;; CHECK-NEXT: (i32.or
219219
;; CHECK-NEXT: (i32.eqz

‎test/lit/passes/optimize-instructions-mvp.wast‎

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5754,15 +5754,13 @@
57545754
;; CHECK-NEXT: )
57555755
;; CHECK-NEXT: )
57565756
;; CHECK-NEXT: (drop
5757-
;; CHECK-NEXT: (block (result i32)
5758-
;; CHECK-NEXT: (i32.add
5759-
;; CHECK-NEXT: (local.get $1)
5760-
;; CHECK-NEXT: (unreachable)
5761-
;; CHECK-NEXT: )
5757+
;; CHECK-NEXT: (i32.add
5758+
;; CHECK-NEXT: (local.get $1)
5759+
;; CHECK-NEXT: (unreachable)
57625760
;; CHECK-NEXT: )
57635761
;; CHECK-NEXT: )
57645762
;; CHECK-NEXT: (drop
5765-
;; CHECK-NEXT: (block (result i32)
5763+
;; CHECK-NEXT: (block
57665764
;; CHECK-NEXT: (drop
57675765
;; CHECK-NEXT: (local.tee $0
57685766
;; CHECK-NEXT: (local.get $1)
@@ -5775,7 +5773,7 @@
57755773
;; CHECK-NEXT: )
57765774
;; CHECK-NEXT: )
57775775
;; CHECK-NEXT: (drop
5778-
;; CHECK-NEXT: (if (result i32)
5776+
;; CHECK-NEXT: (if
57795777
;; CHECK-NEXT: (unreachable)
57805778
;; CHECK-NEXT: (then
57815779
;; CHECK-NEXT: (i32.add
@@ -15718,23 +15716,21 @@
1571815716
)
1571915717
)
1572015718
)
15721-
;; CHECK: (func $if-dont-change-to-unreachable (param $x i32) (param $y i32) (param $z i32) (result i32)
15722-
;; CHECK-NEXT: (if (result i32)
15723-
;; CHECK-NEXT: (local.get $x)
15724-
;; CHECK-NEXT: (then
15725-
;; CHECK-NEXT: (return
15719+
;; CHECK: (func $if-unreachable-return-identical (param $x i32) (param $y i32) (param $z i32) (result i32)
15720+
;; CHECK-NEXT: (return
15721+
;; CHECK-NEXT: (if (result i32)
15722+
;; CHECK-NEXT: (local.get $x)
15723+
;; CHECK-NEXT: (then
1572615724
;; CHECK-NEXT: (local.get $y)
1572715725
;; CHECK-NEXT: )
15728-
;; CHECK-NEXT: )
15729-
;; CHECK-NEXT: (else
15730-
;; CHECK-NEXT: (return
15726+
;; CHECK-NEXT: (else
1573115727
;; CHECK-NEXT: (local.get $z)
1573215728
;; CHECK-NEXT: )
1573315729
;; CHECK-NEXT: )
1573415730
;; CHECK-NEXT: )
1573515731
;; CHECK-NEXT: )
15736-
(func $if-dont-change-to-unreachable (param $x i32) (param $y i32) (param $z i32) (result i32)
15737-
;; if we move the returns outside, we'd become unreachable; avoid that.
15732+
(func $if-unreachable-return-identical (param $x i32) (param $y i32) (param $z i32) (result i32)
15733+
;; We can move the returns outside because we are already unreachable.
1573815734
(if (result i32)
1573915735
(local.get $x)
1574015736
(then

0 commit comments

Comments
 (0)