Skip to content
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.

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

[Wasm EH] Optimize throws caught by TryTable into breaks #6980

Merged
merged 11 commits into from
Oct 3, 2024

Conversation

kripken
Copy link
Member

@kripken kripken commented Oct 1, 2024

E.g.

(try_table (catch_all $catch)
  (throw $e)
)

=>

(try_table (catch_all $catch)
  (br $catch)
)

This can then allow other passes to remove the TryTable, if no throwing
things remain.

@kripken kripken requested a review from aheejin October 1, 2024 18:26
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

LGTM! By the way why would this routine be in RemoveUnusedBr, given that it rather creates branches? Not that I have another place in mind.. (Other than let's say Vacuum which has become a kitchensync of sorts...)

@@ -260,7 +261,7 @@ struct RemoveUnusedBrs : public WalkerPass<PostWalker<RemoveUnusedBrs>> {
} else if (curr->is<Nop>()) {
// ignore (could be result of a previous cycle)
self->stopValueFlow();
} else if (curr->is<Loop>()) {
} else if (curr->is<Loop>()) { // TODO: eh
Copy link
Member

Choose a reason for hiding this comment

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

What does this TODO mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just that we can optimize exception handling here, both Try and TryTable. I'll do that soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

(the handling will be similar to Loop)

src/passes/RemoveUnusedBrs.cpp Outdated Show resolved Hide resolved
@kripken
Copy link
Member Author

kripken commented Oct 2, 2024

By the way why would this routine be in RemoveUnusedBr, given that it rather creates branches?

My reasoning is that by lowering a throw to a break we can then optimize that break right here in the pass, without waiting to reach another pass (e.g. a test here shows a throw turning into a br that turn turns into a br_if).

But, yeah, this could have fit elsewhere. I see Vacuum more as "remove obviously unneeded (but not dead) code", so if I had to pick another place I'd use OptimizeInstructions, though this is a little more complex than the typical peephole optimization there...

Co-authored-by: Heejin Ahn <aheejin@gmail.com>
@aheejin
Copy link
Member

aheejin commented Oct 2, 2024

If a branch is created here, can it be optimized away in the same run of the pass? Apparently it can, as you've shown with the br_if example... Does that mean it traverses the program more than once?

@kripken
Copy link
Member Author

kripken commented Oct 2, 2024

Yes, this does loop over several simplifications, and keeps going until things stop changing. That main loop starts here:

void doWalkFunction(Function* func) {
// multiple cycles may be needed
do {
anotherCycle = false;

This could probably be more efficient, but each traversal is practically linear. This pass didn't show up as slow in any of my recent performance measurements.

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

I see, then it makes sense to put it here. Thanks!

@kripken kripken merged commit ce1a13c into WebAssembly:main Oct 3, 2024
13 checks passed
@kripken kripken deleted the throw.break branch October 3, 2024 15:15
kripken added a commit that referenced this pull request Oct 7, 2024
We ignored legacy Trys in #6980, but they can also catch.
kripken added a commit that referenced this pull request Oct 8, 2024
#6980 was missing the logic to reset flows after replacing a throw. The process
of replacing the throw introduces new code and in particular a drop, which
blocks branches from flowing to their targets.

In the testcase here, the br was turned into nop before this fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants