Skip to content

Fix the case of an unreachable entry in LocalGraph #6048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Fix the case of an unreachable entry in LocalGraph #6048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb4a846
fix
kripken Oct 24, 2023
bb995af
proper fix
kripken Oct 24, 2023
f247b98
fix
kripken Oct 24, 2023
9998c10
comment
kripken Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions src/ir/LocalGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ struct Info {
std::vector<Expression*> actions;
// for each index, the last local.set for it
std::unordered_map<Index, LocalSet*> lastSets;

void dump(Function* func) {
std::cout << " info: " << actions.size() << " actions\n";
}
};

// flow helper class. flows the gets to their sets
Expand Down Expand Up @@ -106,10 +110,6 @@ struct Flower : public CFGWalker<Flower, Visitor<Flower>, Info> {
auto numLocals = func->getNumLocals();
std::vector<FlowBlock*> work;

// Track if we have unreachable code anywhere, as if we do that may inhibit
// certain optimizations below.
bool hasUnreachable = false;

// Convert input blocks (basicBlocks) into more efficient flow blocks to
// improve memory access.
std::vector<FlowBlock> flowBlocks;
Expand All @@ -120,11 +120,6 @@ struct Flower : public CFGWalker<Flower, Visitor<Flower>, Info> {
for (Index i = 0; i < basicBlocks.size(); ++i) {
auto* block = basicBlocks[i].get();
basicToFlowMap[block] = &flowBlocks[i];
// Check for unreachable code. Note we ignore the entry block (index 0) as
// that is always reached when we are called.
if (i != 0 && block->in.empty()) {
hasUnreachable = true;
}
}

// We note which local indexes have local.sets, as that can help us
Expand Down Expand Up @@ -200,19 +195,16 @@ struct Flower : public CFGWalker<Flower, Visitor<Flower>, Info> {
if (gets.empty()) {
continue;
}
if (!hasUnreachable && !hasSet[index]) {
if (!hasSet[index]) {
// This local index has no sets, so we know all gets will end up
// reaching the entry block. Do that here as an optimization to avoid
// flowing through the (potentially very many) blocks in the function.
//
// Note that we must check for unreachable code in this function, as
// if there is any then we would not be precise: in that case, the
// gets may either have the entry value, or no value at all. It would
// be safe to mark the entry value in that case anyhow (as it only
// matters in unreachable code), but to keep the IR consistent and to
// avoid confusion when debugging, simply do not optimize if
// there is anything unreachable (which will not happen normally, as
// DCE should run before passes that use this utility).
// Note that we may be in unreachable code, and if so, we might add
// the entry values when they are not actually relevant. That is, we
// are not precise in the case of unreachable code. This can be
// confusing when debugging, but it does not have any downside for
// optimization (since unreachable code should be removed anyhow).
for (auto* get : gets) {
getSetses[get].insert(nullptr);
}
Expand Down
8 changes: 8 additions & 0 deletions src/ir/local-graph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ namespace wasm {
// (see the SSA pass for actually creating new local indexes based
// on this).
//
// Note that this is not guaranteed to be precise in unreachable code. That is,
// we do not make the effort to represent the exact sets for each get, and may
// overestimate them (specifically, we may mark the entry value as possible,
// even if unreachability prevents that; doing so helps simplify and optimize
// the code, which we think is worthwhile for the possible annoyance in
// debugging etc.; and it has no downside for optimization, since unreachable
// code will be removed anyhow).
//
struct LocalGraph {
// main API

Expand Down
14 changes: 11 additions & 3 deletions src/passes/Precompute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,18 @@ struct Precompute
if (getFunction()->isVar(get->index)) {
auto localType = getFunction()->getLocalType(get->index);
if (localType.isNonNullable()) {
Fatal() << "Non-nullable local accessing the default value in "
<< getFunction()->name << " (" << get->index << ')';
// This is a non-nullable local that seems to read the default
// value at the function entry. This is either an internal error
// or a case of unreachable code; the latter is possible as
// LocalGraph is not precise in unreachable code.
//
// We cannot set zeros here (as applying them, even in
// unreachable code, would not validate), so just mark this as
// a hopeless case to ignore.
values = {};
} else {
curr = Literal::makeZeros(localType);
}
curr = Literal::makeZeros(localType);
} else {
// it's a param, so it's hopeless
values = {};
Expand Down
91 changes: 83 additions & 8 deletions test/lit/passes/precompute-gc.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@
(struct.get $struct 0 (local.get $x))
)
)
;; CHECK: (func $ref-comparisons (type $9) (param $x (ref null $struct)) (param $y (ref null $struct))
;; CHECK: (func $ref-comparisons (type $10) (param $x (ref null $struct)) (param $y (ref null $struct))
;; CHECK-NEXT: (local $z (ref null $struct))
;; CHECK-NEXT: (local $w (ref null $struct))
;; CHECK-NEXT: (call $log
Expand Down Expand Up @@ -407,7 +407,7 @@
(local.get $tempresult)
)

;; CHECK: (func $propagate-different-params (type $10) (param $input1 (ref $empty)) (param $input2 (ref $empty)) (result i32)
;; CHECK: (func $propagate-different-params (type $11) (param $input1 (ref $empty)) (param $input2 (ref $empty)) (result i32)
;; CHECK-NEXT: (local $tempresult i32)
;; CHECK-NEXT: (local.set $tempresult
;; CHECK-NEXT: (ref.eq
Expand Down Expand Up @@ -723,7 +723,7 @@
)
)

;; CHECK: (func $helper (type $11) (param $0 i32) (result i32)
;; CHECK: (func $helper (type $12) (param $0 i32) (result i32)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
(func $helper (param i32) (result i32)
Expand Down Expand Up @@ -801,14 +801,14 @@
)
)

;; CHECK: (func $receive-f64 (type $12) (param $0 f64)
;; CHECK: (func $receive-f64 (type $13) (param $0 f64)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
(func $receive-f64 (param f64)
(unreachable)
)

;; CHECK: (func $odd-cast-and-get-non-null (type $13) (param $temp (ref $func-return-i32))
;; CHECK: (func $odd-cast-and-get-non-null (type $14) (param $temp (ref $func-return-i32))
;; CHECK-NEXT: (local.set $temp
;; CHECK-NEXT: (ref.cast (ref nofunc)
;; CHECK-NEXT: (ref.func $receive-f64)
Expand Down Expand Up @@ -857,7 +857,7 @@
)
)

;; CHECK: (func $br_on_cast-on-creation (type $14) (result (ref $empty))
;; CHECK: (func $br_on_cast-on-creation (type $15) (result (ref $empty))
;; CHECK-NEXT: (block $label (result (ref $empty))
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (br_on_cast $label (ref $empty) (ref $empty)
Expand Down Expand Up @@ -952,7 +952,7 @@
)
)

;; CHECK: (func $remove-set (type $15) (result (ref func))
;; CHECK: (func $remove-set (type $16) (result (ref func))
;; CHECK-NEXT: (local $nn funcref)
;; CHECK-NEXT: (local $i i32)
;; CHECK-NEXT: (loop $loop
Expand Down Expand Up @@ -995,7 +995,7 @@
)
)

;; CHECK: (func $strings (type $16) (param $param (ref string))
;; CHECK: (func $strings (type $17) (param $param (ref string))
;; CHECK-NEXT: (local $s (ref string))
;; CHECK-NEXT: (local.set $s
;; CHECK-NEXT: (string.const "hello, world")
Expand Down Expand Up @@ -1066,4 +1066,79 @@
)
(local.get $x)
)

;; CHECK: (func $get-nonnullable-in-unreachable-entry (type $9) (param $x i32) (param $y (ref any))
;; CHECK-NEXT: (local $0 (ref any))
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: )
;; CHECK-NEXT: (loop $loop
;; CHECK-NEXT: (br_if $loop
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $get-nonnullable-in-unreachable-entry (param $x i32) (param $y (ref any))
(local $0 (ref any))
;; As above, but now the first basic block is unreachable, and we need to
;; detect that specifically, as the block after it *does* have entries even
;; though it is unreachable (it is a loop, and has itself as an entry).
(unreachable)
(local.set $0
(local.get $y)
)
(loop $loop
(br_if $loop
(local.get $x)
)
(drop
(local.get $0)
)
)
)

;; CHECK: (func $get-nonnullable-in-unreachable-later-loop (type $9) (param $x i32) (param $y (ref any))
;; CHECK-NEXT: (local $0 (ref any))
;; CHECK-NEXT: (if
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: )
;; CHECK-NEXT: (loop $loop
;; CHECK-NEXT: (br_if $loop
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $get-nonnullable-in-unreachable-later-loop (param $x i32) (param $y (ref any))
(local $0 (ref any))
;; This |if| is added, which means the loop is later in the function.
;; Otherwise this is the same as before.
(if
(local.get $x)
(nop)
)
(unreachable)
(local.set $0
(local.get $y)
)
(loop $loop
(br_if $loop
(local.get $x)
)
(drop
(local.get $0)
)
)
)
)