Skip to content

Commit

Permalink
Make more Ifs unreachable (#7094)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tlively
tlively authored Nov 27, 2024
1 parent cd3805e commit 6f0f2e0
Show file tree
Hide file tree
Showing 19 changed files with 282 additions and 108 deletions.
2 changes: 1 addition & 1 deletion src/ir/subtype-exprs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ struct SubtypingDiscoverer : public OverriddenVisitor<SubType> {
}
}
void visitIf(If* curr) {
if (curr->ifFalse) {
if (curr->ifFalse && curr->type != Type::unreachable) {
self()->noteSubtype(curr->ifTrue, curr);
self()->noteSubtype(curr->ifFalse, curr);
}
Expand Down
7 changes: 7 additions & 0 deletions src/passes/CodeFolding.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,13 @@ struct CodeFolding
if (!curr->ifFalse) {
return;
}
if (curr->condition->type == Type::unreachable) {
// If the arms are foldable and concrete, we would be replacing an
// unreachable If with a concrete block, which may or may not be valid,
// depending on the context. Leave this for DCE rather than trying to
// handle that.
return;
}
// If both are blocks, look for a tail we can merge.
auto* left = curr->ifTrue->dynCast<Block>();
auto* right = curr->ifFalse->dynCast<Block>();
Expand Down
16 changes: 10 additions & 6 deletions src/passes/Flatten.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,20 +147,24 @@ struct Flatten
// arm preludes go in the arms. we must also remove an if value
auto* originalIfTrue = iff->ifTrue;
auto* originalIfFalse = iff->ifFalse;
auto type = iff->type;
auto type = iff->ifFalse ? Type::getLeastUpperBound(iff->ifTrue->type,
iff->ifFalse->type)
: Type::none;
Expression* prelude = nullptr;
if (type.isConcrete()) {
Index temp = builder.addVar(getFunction(), type);
if (iff->ifTrue->type.isConcrete()) {
iff->ifTrue = builder.makeLocalSet(temp, iff->ifTrue);
}
if (iff->ifFalse && iff->ifFalse->type.isConcrete()) {
if (iff->ifFalse->type.isConcrete()) {
iff->ifFalse = builder.makeLocalSet(temp, iff->ifFalse);
}
// the whole if (+any preludes from the condition) is now a prelude
prelude = rep;
// and we leave just a get of the value
rep = builder.makeLocalGet(temp, type);
if (curr->type.isConcrete()) {
// the whole if (+any preludes from the condition) is now a prelude
prelude = rep;
// and we leave just a get of the value
rep = builder.makeLocalGet(temp, type);
}
}
iff->ifTrue = getPreludesWithExpression(originalIfTrue, iff->ifTrue);
if (iff->ifFalse) {
Expand Down
11 changes: 9 additions & 2 deletions src/passes/Print.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,9 +460,16 @@ struct PrintExpressionContents
}
void visitIf(If* curr) {
printMedium(o, "if");
if (curr->type.isConcrete()) {
// Ifs are unreachable if their condition is unreachable, but in that case
// the arms might have some concrete type we have to account for to produce
// valid wat.
auto type = curr->type;
if (curr->condition->type == Type::unreachable && curr->ifFalse) {
type = Type::getLeastUpperBound(curr->ifTrue->type, curr->ifFalse->type);
}
if (type.isConcrete()) {
o << ' ';
printBlockType(Signature(Type::none, curr->type));
printBlockType(Signature(Type::none, type));
}
}
void visitLoop(Loop* curr) {
Expand Down
23 changes: 10 additions & 13 deletions src/wasm/wasm-validator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,16 @@ void FunctionValidator::visitIf(If* curr) {
curr,
"returning if-else's false must have right type");
} else {
if (curr->condition->type != Type::unreachable) {
if (curr->condition->type == Type::unreachable) {
shouldBeTrue(
curr->ifTrue->type == Type::unreachable ||
curr->ifFalse->type == Type::unreachable ||
(curr->ifTrue->type == Type::none &&
curr->ifFalse->type == Type::none) ||
Type::hasLeastUpperBound(curr->ifTrue->type, curr->ifFalse->type),
curr,
"arms of unreachable if-else must have compatible types");
} else {
shouldBeEqual(curr->ifTrue->type,
Type(Type::unreachable),
curr,
Expand All @@ -876,18 +885,6 @@ void FunctionValidator::visitIf(If* curr) {
"unreachable if-else must have unreachable false");
}
}
if (curr->ifTrue->type.isConcrete()) {
shouldBeSubType(curr->ifTrue->type,
curr->type,
curr,
"if type must match concrete ifTrue");
}
if (curr->ifFalse->type.isConcrete()) {
shouldBeSubType(curr->ifFalse->type,
curr->type,
curr,
"if type must match concrete ifFalse");
}
}
}

Expand Down
30 changes: 11 additions & 19 deletions src/wasm/wasm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,28 +215,20 @@ void Block::finalize(std::optional<Type> type_, Breakability breakability) {
}

void If::finalize(std::optional<Type> type_) {
if (type_) {
type = *type_;
if (type == Type::none && (condition->type == Type::unreachable ||
(ifFalse && ifTrue->type == Type::unreachable &&
ifFalse->type == Type::unreachable))) {
type = Type::unreachable;
}
// The If is unreachable if the condition is unreachable or both arms are
// unreachable.
if (condition->type == Type::unreachable ||
(ifFalse && ifTrue->type == Type::unreachable &&
ifFalse->type == Type::unreachable)) {
type = Type::unreachable;
return;
}

type = ifFalse ? Type::getLeastUpperBound(ifTrue->type, ifFalse->type)
: Type::none;
// if the arms return a value, leave it even if the condition
// is unreachable, we still mark ourselves as having that type, e.g.
// (if (result i32)
// (unreachable)
// (i32.const 10)
// (i32.const 20)
// )
// otherwise, if the condition is unreachable, so is the if
if (type == Type::none && condition->type == Type::unreachable) {
type = Type::unreachable;
if (type_) {
type = *type_;
} else {
type = ifFalse ? Type::getLeastUpperBound(ifTrue->type, ifFalse->type)
: Type::none;
}
}

Expand Down
44 changes: 26 additions & 18 deletions test/lit/passes/code-folding.wast
View file
Original file line number Diff line number Diff line change
Expand Up @@ -743,12 +743,20 @@
)

;; CHECK: (func $unreachable-if (type $0)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $unreachable-if
Expand All @@ -773,14 +781,17 @@
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $unreachable-if-suffix
(if
Expand All @@ -800,16 +811,13 @@
)

;; CHECK: (func $unreachable-if-concrete-arms (type $0)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (then
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
Expand Down
12 changes: 3 additions & 9 deletions test/lit/passes/flatten_all-features.wast
View file
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,6 @@
;; CHECK-NEXT: (local $1 i32)
;; CHECK-NEXT: (local $2 i32)
;; CHECK-NEXT: (local $3 i32)
;; CHECK-NEXT: (local $4 i32)
;; CHECK-NEXT: (block $x
;; CHECK-NEXT: (block
;; CHECK-NEXT: (local.set $0
Expand All @@ -646,18 +645,13 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $3
;; CHECK-NEXT: (local.get $2)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (local.get $3)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $4
;; CHECK-NEXT: (local.set $3
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (return
;; CHECK-NEXT: (local.get $4)
;; CHECK-NEXT: (local.get $3)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $a13 (result i32)
Expand Down
2 changes: 1 addition & 1 deletion test/lit/passes/optimize-instructions-ignore-traps.wast
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.tee $0
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (i32.or
;; CHECK-NEXT: (i32.eqz
Expand Down
30 changes: 13 additions & 17 deletions test/lit/passes/optimize-instructions-mvp.wast
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5754,15 +5754,13 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (block
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.tee $0
;; CHECK-NEXT: (local.get $1)
Expand All @@ -5775,7 +5773,7 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (if
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (i32.add
Expand Down Expand Up @@ -15718,23 +15716,21 @@
)
)
)
;; CHECK: (func $if-dont-change-to-unreachable (param $x i32) (param $y i32) (param $z i32) (result i32)
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (return
;; CHECK: (func $if-unreachable-return-identical (param $x i32) (param $y i32) (param $z i32) (result i32)
;; CHECK-NEXT: (return
;; CHECK-NEXT: (if (result i32)
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (return
;; CHECK-NEXT: (else
;; CHECK-NEXT: (local.get $z)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $if-dont-change-to-unreachable (param $x i32) (param $y i32) (param $z i32) (result i32)
;; if we move the returns outside, we'd become unreachable; avoid that.
(func $if-unreachable-return-identical (param $x i32) (param $y i32) (param $z i32) (result i32)
;; We can move the returns outside because we are already unreachable.
(if (result i32)
(local.get $x)
(then
Expand Down
40 changes: 40 additions & 0 deletions test/lit/passes/unsubtyping.wast
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1815,3 +1815,43 @@
)
)
)

;; Regression test for a crash on ifs with unreachable conditions and tuple arms.
(module
;; CHECK: (type $0 (func (result i32 i64)))

;; CHECK: (func $test (type $0) (result i32 i64)
;; CHECK-NEXT: (if (type $0) (result i32 i64)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (tuple.make 2
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: (i64.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (tuple.make 2
;; CHECK-NEXT: (i32.const 2)
;; CHECK-NEXT: (i64.const 3)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test (result i32 i64)
(if (result i32 i64)
(unreachable)
(then
(tuple.make 2
(i32.const 0)
(i64.const 1)
)
)
(else
(tuple.make 2
(i32.const 2)
(i64.const 3)
)
)
)
)
)
Loading

0 comments on commit 6f0f2e0

Please sign in to comment.