Skip to content

Handle try in Flatten pass #2567

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 9 commits into from
Nov 29, 2021
Merged

Handle try in Flatten pass #2567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fdbbd74
Handle try in Flatten pass
aheejin Jan 3, 2020
834155a
Merge branch 'main' into flatten_try
aheejin Nov 22, 2021
d58252e
New spec + test
aheejin Nov 22, 2021
574121c
Merge branch 'main' into flatten_try
aheejin Nov 22, 2021
8fb0c89
Filecheck output
aheejin Nov 22, 2021
111175b
Address comments
aheejin Nov 22, 2021
503d2bb
Remove redundant finalization from if/try
aheejin Nov 23, 2021
3725b1b
Fix try + restore if's finalization
aheejin Nov 23, 2021
d1e6f9b
Add unreachable tests
aheejin Nov 23, 2021
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
4 changes: 4 additions & 0 deletions src/ir/eh-utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ bool isPopValid(Expression* catchBody) {
}

void handleBlockNestedPops(Function* func, Module& wasm) {
if (!wasm.features.hasExceptionHandling()) {
return;
}

Builder builder(wasm);
FindAll<Try> trys(func->body);
for (auto* try_ : trys.list) {
Expand Down
36 changes: 36 additions & 0 deletions src/passes/Flatten.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@

#include <ir/branch-utils.h>
#include <ir/effects.h>
#include <ir/eh-utils.h>
#include <ir/flat.h>
#include <ir/properties.h>
#include <ir/type-updating.h>
Expand Down Expand Up @@ -192,6 +193,37 @@ struct Flatten
loop->finalize();
replaceCurrent(rep);

} else if (auto* tryy = curr->dynCast<Try>()) {
// remove a try value
Expression* rep = tryy;
auto* originalBody = tryy->body;
std::vector<Expression*> originalCatchBodies(tryy->catchBodies.begin(),
tryy->catchBodies.end());
auto type = tryy->type;
if (type.isConcrete()) {
Index temp = builder.addVar(getFunction(), type);
if (tryy->body->type.isConcrete()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this if just skips adding a set if the body is unreachable, as a minor optimization? I don't think it's needed IIUC. And the catch bodies do not do this check which makes it a little surprising to see it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copy-pasted if's code here again:

binaryen/src/passes/Flatten.cpp

Lines 152 to 159 in 3c1c8ae

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()) {
iff->ifFalse = builder.makeLocalSet(temp, iff->ifFalse);
}

I tried to remove these if (iff->ifTrue->type.isConcrete()) and if (iff->ifFalse && iff->ifFalse->type.isConcrete()) to see if these also can be removed, but if I do that these test fail:

Failed Tests (2):
  Binaryen lit tests :: passes/flatten_simplify-locals-nonesting_souperify-single-use_enable-threads.wast
  Binaryen lit tests :: passes/flatten_simplify-locals-nonesting_souperify_enable-threads.wast

I haven't checked why they fail (and I have to run for today), but I guess there can be cases these are necessary...? If that's the case I think I should also check the catch bodies too. I'll do that for the moment, and if they turn out not to be necessary, I will remove them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks. In that case let's check the catch bodies too, that seems most consistent with the rest of the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to remove these body checks (both in if and try); the reason those tests failed was not because they crashed, but because the filecheck results were different. As you said, this does a small optimization that prevents this form:

(local.set $x
  (unreachable)
)

But I think it's OK to leave them (both in if and try) anyway; while Flatten doesn't try to do any optimizations itself, it wouldn't hurt, and maybe the output will be a little easier to read...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm, this simple pattern doesn't add much code complexity and it does make the output smaller and simpler.

tryy->body = builder.makeLocalSet(temp, tryy->body);
}
for (Index i = 0; i < tryy->catchBodies.size(); i++) {
if (tryy->catchBodies[i]->type.isConcrete()) {
tryy->catchBodies[i] =
builder.makeLocalSet(temp, tryy->catchBodies[i]);
}
}
// and we leave just a get of the value
rep = builder.makeLocalGet(temp, type);
// the whole try is now a prelude
ourPreludes.push_back(tryy);
}
tryy->body = getPreludesWithExpression(originalBody, tryy->body);
for (Index i = 0; i < tryy->catchBodies.size(); i++) {
tryy->catchBodies[i] = getPreludesWithExpression(
originalCatchBodies[i], tryy->catchBodies[i]);
}
tryy->finalize();
replaceCurrent(rep);

} else {
WASM_UNREACHABLE("unexpected expr type");
}
Expand Down Expand Up @@ -347,6 +379,10 @@ struct Flatten
<< type;
}
}

// Flatten can generate blocks within 'catch', making pops invalid. Fix them
// up.
EHUtils::handleBlockNestedPops(curr, *getModule());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like handleBlockNestedPops will scan for Trys without checking the features first. How about adding an early exit there if exceptions are not enabled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at the beginning of handleBlockNestedPops.

}

private:
Expand Down
234 changes: 234 additions & 0 deletions test/lit/passes/flatten-eh.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
;; RUN: wasm-opt %s -all --flatten -S -o - | filecheck %s

(module
;; CHECK: (tag $e-i32 (param i32))
(tag $e-i32 (param i32))
;; CHECK: (tag $e-f32 (param f32))
(tag $e-f32 (param f32))

;; CHECK: (func $try_catch
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (local $1 i32)
;; CHECK-NEXT: (local $2 f32)
;; CHECK-NEXT: (try $try
;; CHECK-NEXT: (do
;; CHECK-NEXT: (throw $e-i32
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-i32
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-f32
;; CHECK-NEXT: (local.set $2
;; CHECK-NEXT: (pop f32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.get $2)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $try_catch (local $x i32)
;; Basic try-catch test
(try
(do
(throw $e-i32 (i32.const 0))
)
(catch $e-i32
(drop
(pop i32)
)
)
(catch $e-f32
(drop
(pop f32)
)
)
)
)

;; CHECK: (func $try_catch_pop_fixup
;; CHECK-NEXT: (local $0 i32)
;; CHECK-NEXT: (local $1 i32)
;; CHECK-NEXT: (block $l0
;; CHECK-NEXT: (try $try
;; CHECK-NEXT: (do
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-i32
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (block
;; CHECK-NEXT: (block
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (br $l0)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $try_catch_pop_fixup
;; After --flatten, a block is created within the 'catch', which makes the
;; pop's location invalid. This tests whether it is fixed up correctly after
;; --flatten.
(block $l0
(try
(do)
(catch $e-i32
(drop
(pop i32)
)
(br $l0)
)
)
)
)

;; CHECK: (func $try_catch_return_value (result i32)
;; CHECK-NEXT: (local $0 i32)
;; CHECK-NEXT: (local $1 i32)
;; CHECK-NEXT: (local $2 i32)
;; CHECK-NEXT: (try $try
;; CHECK-NEXT: (do
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-i32
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $2
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (return
;; CHECK-NEXT: (local.get $2)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $try_catch_return_value (result i32)
(try (result i32)
(do
(i32.const 0)
)
(catch $e-i32
(pop i32)
)
)
)

;; CHECK: (func $try_unreachable (result i32)
;; CHECK-NEXT: (local $0 i32)
;; CHECK-NEXT: (local $1 i32)
;; CHECK-NEXT: (local $2 i32)
;; CHECK-NEXT: (try $try
;; CHECK-NEXT: (do
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-i32
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $1
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $2
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (return
;; CHECK-NEXT: (local.get $2)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $try_unreachable (result i32)
(try (result i32)
(do
(unreachable)
)
(catch $e-i32
(pop i32)
)
)
)

;; CHECK: (func $catch_unreachable (result i32)
;; CHECK-NEXT: (local $0 i32)
;; CHECK-NEXT: (local $1 i32)
;; CHECK-NEXT: (local $2 i32)
;; CHECK-NEXT: (local $3 i32)
;; CHECK-NEXT: (local $4 i32)
;; CHECK-NEXT: (local $5 i32)
;; CHECK-NEXT: (try $try
;; CHECK-NEXT: (do
;; CHECK-NEXT: (local.set $3
;; CHECK-NEXT: (i32.const 3)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (catch $e-i32
;; CHECK-NEXT: (local.set $5
;; CHECK-NEXT: (pop i32)
;; CHECK-NEXT: )
;; CHECK-NEXT: (block
;; CHECK-NEXT: (block
;; CHECK-NEXT: (local.set $0
;; CHECK-NEXT: (local.get $5)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (local.get $0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $2
;; CHECK-NEXT: (local.get $1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $3
;; CHECK-NEXT: (local.get $2)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.set $4
;; CHECK-NEXT: (local.get $3)
;; CHECK-NEXT: )
;; CHECK-NEXT: (return
;; CHECK-NEXT: (local.get $4)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $catch_unreachable (result i32)
(try (result i32)
(do
(i32.const 3)
)
(catch $e-i32
(drop
(pop i32)
)
(unreachable)
)
)
)
)